feat: use withDefaults for AssigneeList #2572

dpschen · 2024-07-16T18:59:17Z

dpschen commented

2024-07-16 18:59:17 +00:00

No description provided.

dpschen added 1 commit 2024-07-16 18:59:19 +00:00

feat: use withDefaults for AssigneeList

continuous-integration/drone/pr Build is failing

Details

e295be0a35

frederick commented

2024-07-16 19:12:28 +00:00

Hi dpschen!

Thank you for creating a PR!

I've deployed the frontend changes of this PR on a preview environment under this URL: https://2572-feat-withdefaults-for-assigneeli--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somewhere. The easiest to use is https://try.vikunja.io/.

This preview does not contain any changes made to the api, only the frontend.

Have a nice day!

Beep boop, I'm a bot.

Hi dpschen! Thank you for creating a PR! I've deployed the frontend changes of this PR on a preview environment under this URL: https://2572-feat-withdefaults-for-assigneeli--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somewhere. The easiest to use is https://try.vikunja.io/. This preview does not contain any changes made to the api, only the frontend. Have a nice day! > Beep boop, I'm a bot.

konrad commented

2024-07-16 21:35:43 +00:00

Tests are now failing

dpschen force-pushed feat/withDefaults-for-AssigneeList from e295be0a35 to 5019ded6a6

2024-07-18 15:45:56 +00:00

Compare

konrad reviewed 2024-07-19 05:30:09 +00:00

frontend/src/components/tasks/partials/AssigneeList.vue

						
				@ -19,3 +11,1 @@

				}>()

				const hasDelete = computed(() => typeof remove !== 'undefined' && !disabled)

					onRemove?: (user: IUser) => void

konrad commented

2024-07-19 05:30:09 +00:00

Shouldn't this be an emit? Since you're using @remove in the parent

dpschen commented

2024-07-19 08:59:12 +00:00

I looked really into this and am also not happy with the solution. Maybe the original solution was better after all 😂:

Vue.js tries to follow the Pub/Sub pattern closely. This means that in theory you can't check if there is a listener because you only publish (as a component).

There is a workaround which — because it's not intended — seemed even dirtier than the proposed solution:

const useHasListener = (name: `on${string}`) => {
	return computed(() => !!getCurrentInstance()?.vnode.props?.[name]
}

const hasRemoveListener = useHasListener('onRemove')

Given this and the fact that every prop that starts with on is treated like an event from the parent components perspective it seemed better in the moment to create a prop with on and use it as I did.

Now I'm not so sure anymore. What do you think? Quite a lot vue plugins make use of getCurrentInstance so maybe it's not so bad after all?

I looked really into this and am also not happy with the solution. Maybe the original solution was better after all 😂: Vue.js tries to follow the Pub/Sub pattern closely. This means that in theory you can't check if there is a listener because you only publish (as a component). There is a workaround which — because it's not intended — seemed even dirtier than the proposed solution: ```ts const useHasListener = (name: `on${string}`) => { return computed(() => !!getCurrentInstance()?.vnode.props?.[name] } const hasRemoveListener = useHasListener('onRemove') ``` Given this and the fact that every prop that starts with `on` is treated like an event from the parent components perspective it seemed better in the moment to create a prop with on and use it as I did. Now I'm not so sure anymore. What do you think? Quite a lot vue plugins make use of `getCurrentInstance` so maybe it's not so bad after all?

konrad commented

2024-07-19 09:15:28 +00:00

I think getCurrentInstance doesn't look too bad - it's not a clean solution but passing functions as event handlers via props seems worse since this is generally an antipattern in vue - I know I am guilty of this myself, since I introduced the problem we're discussing here :)

I think `getCurrentInstance` doesn't look too bad - it's not a clean solution but passing functions as event handlers via props seems worse since this is generally an antipattern in vue - I know I am guilty of this myself, since I introduced the problem we're discussing here :)

👍 1

dpschen commented

2024-07-19 09:21:10 +00:00

Okay, will do it that way :)

continuous-integration/drone/pr Build is passing

Required

Details

This pull request doesn't have enough required approvals yet. 0 of 1 approvals granted from users or teams on the allowlist.

This branch is out-of-date with the base branch

You are not authorized to merge this pull request.

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u feat/withDefaults-for-AssigneeList:dpschen-feat/withDefaults-for-AssigneeList

git checkout dpschen-feat/withDefaults-for-AssigneeList

Sign in to join this conversation.

3 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/vikunja#2572