fix: saving default list #1143

konrad · 2021-12-05T14:32:55Z

konrad commented

2021-12-05 14:32:55 +00:00

Relevant forum discussion: https://community.vikunja.io/t/quick-add-magic-for-list-not-working/493/3

konrad added 1 commit 2021-12-05 14:32:55 +00:00

fix: saving default list

continuous-integration/drone/pr Build is passing

Details

0aafbd34a8

konrad reviewed 2021-12-05 14:33:27 +00:00

src/views/user/settings/General.vue

						
				@ -183,0 +184,4 @@

							get() {

								return this.$store.getters['lists/getListById'](this.settings.defaultListId)

							},

							set(l) {

konrad commented

2021-12-05 14:33:27 +00:00

It looks like this does not even generate an error message.

@dpschen Any idea why?

It looks like this does not even generate an error message. @dpschen Any idea why?

dpschen commented

2021-12-06 13:11:49 +00:00

Why should it generate one? I think I need more context.

Regardless:
This seems like the wrong location to set a default value. 0 isn't recognizable as such.
I think at least it should be a const (e.g. DEFAULT_LIST_ID).

Maybe better: creating a defaultList getter in the store and use that everywhere where we need the defaultList. Because else we will repeat the returning of the defaultListId as 0 over and over =)

Why should it generate one? I think I need more context. Regardless: This seems like the wrong location to set a default value. `0` isn't recognizable as such. I think at least it should be a const (e.g. `DEFAULT_LIST_ID`). Maybe better: creating a defaultList getter in the store and use that everywhere where we need the defaultList. Because else we will repeat the returning of the defaultListId as `0` over and over =)

konrad commented

2021-12-07 20:08:16 +00:00

Why should it generate one? I think I need more context.

Because I'm trying to assign to a computed value. It works with a setter, but doesn't this should throw an error?

Maybe better: creating a defaultList getter in the store and use that everywhere where we need the defaultList. Because else we will repeat the returning of the defaultListId as 0 over and over =)

But here in this case I want to query the set default list from the component instead of the one currently saved in the store. Having a getter in the store with a function property that's only used once seems a bit pointless.

Not really sure how to improve this. Added a DEFAULT_LIST_ID const for now, maybe that already improves things a bit.

> Why should it generate one? I think I need more context. Because I'm trying to assign to a computed value. It works with a setter, but doesn't this should throw an error? > Maybe better: creating a defaultList getter in the store and use that everywhere where we need the defaultList. Because else we will repeat the returning of the defaultListId as 0 over and over =) But here in this case I want to query the set default list from the component instead of the one currently saved in the store. Having a getter in the store with a function property that's only used once seems a bit pointless. Not really sure how to improve this. Added a `DEFAULT_LIST_ID` const for now, maybe that already improves things a bit.

dpschen commented

2021-12-07 21:47:46 +00:00

So if you set the defaultListId to 0 that means there is none, right?

EDIT: in the sense of: we reset the defaultListId

If that's the reason for the 0 then maybe we should just use that value when sending to the service.
Or even better using this as a formatter inside the service.

So if userSettingsService.update is called with an settings object where defaultListId is undefined just replace that with defaultListId: 0 before sending to the server (?)

So if you set the defaultListId to `0` that means there is none, right? EDIT: in the sense of: we reset the defaultListId If that's the reason for the `0` then maybe we should just use that value when sending to the service. Or even better using this as a formatter inside the service. So if `userSettingsService.update` is called with an settings object where `defaultListId` is undefined just replace that with `defaultListId: 0` before sending to the server (?)

konrad requested review from dpschen 2021-12-05 14:33:33 +00:00

frederick commented

2021-12-05 14:37:15 +00:00

Hi konrad!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://1143-fixtask-create-list--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 somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi konrad! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://1143-fixtask-create-list--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 somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.

konrad added 1 commit 2021-12-05 14:47:19 +00:00

fix: list not set initially

continuous-integration/drone/pr Build is passing

Details

86e827f93c

konrad reviewed 2021-12-05 14:48:12 +00:00

src/components/tasks/partials/listSearch.vue Outdated

						
				@ -38,2 +38,4 @@

						Multiselect,

					},

					mounted() {

						this.list = this.modelValue

konrad commented

2021-12-05 14:48:12 +00:00

It looks like the watcher does not get called, despite immediate set to true.
@dpschen Do you have an idea why that is the case?

It looks like the watcher does not get called, despite `immediate` set to `true`. @dpschen Do you have an idea why that is the case?

dpschen commented

2021-12-06 13:14:11 +00:00

See the other comment

dpschen marked this conversation as resolved

konrad added 1 commit 2021-12-05 15:12:06 +00:00

feat: convert list search component to script setup api

continuous-integration/drone/pr Build was killed

Details

057e847e39

konrad added 1 commit 2021-12-05 15:13:44 +00:00

fix: multiselect search result padding

continuous-integration/drone/pr Build is failing

Details

42ecf4e10a

konrad commented

2021-12-06 07:00:46 +00:00

Note to self: imrove the error message when the list was not found (no default list set or none provided via quick add magic) to tell the users they need to provide a list

dpschen reviewed 2021-12-06 12:57:55 +00:00

dpschen requested changes 2021-12-06 13:14:16 +00:00

src/components/input/multiselect.vue Outdated

						
				@ -458,3 +458,3 @@

							font-family: $family-sans-serif;

							font-weight: normal;

							padding: .5rem 0;

							padding: .5rem .5rem;

dpschen commented

2021-12-06 12:59:42 +00:00

padding: .5rem;

`padding: .5rem;`

konrad commented

2021-12-07 19:39:08 +00:00

Done.

konrad marked this conversation as resolved

src/components/tasks/partials/listSearch.vue Outdated

						
				@ -2,3 +2,3 @@

					<multiselect

						class="control is-expanded"

						:placeholder="$t('list.search')"

						:placeholder="t('list.search')"

dpschen commented

2021-12-06 12:59:17 +00:00

$t is undefined?

`$t` is undefined?

konrad commented

2021-12-07 19:41:41 +00:00

It actually isn't! I just thought I needed to use t since I'm importing it already. Changed it.

It actually isn't! I just thought I needed to use `t` since I'm importing it already. Changed it.

dpschen commented

2021-12-07 21:38:32 +00:00

I'm not sure what way is actually the better one because later we might need to change everything in t or write our own injection as far as I can remeber the vue-i18n 9 documentation.

I'm not sure what way is actually the better one because later we might need to change everything in `t` or write our own injection as far as I can remeber the vue-i18n 9 documentation.

konrad marked this conversation as resolved

src/components/tasks/partials/listSearch.vue Outdated

						
				@ -39,0 +38,4 @@

				watch(

					() => props.modelValue,

					value => {

						list = value

dpschen commented

2021-12-06 13:02:04 +00:00

I think you need to use Object.assign for a reactive here. Because else you will replace the reactive with the static data coming from the prop modelValue.

Or use a ref. But then you need to assign to list.value

I think you need to use `Object.assign` for a reactive here. Because else you will replace the reactive with the static data coming from the prop `modelValue`. Or use a `ref`. But then you need to assign to `list.value`

dpschen commented

2021-12-06 15:18:31 +00:00

If you use watchEffect here you can save the definition of what to watch (vue does that automatically). Also it runs initially by default, so no need for immediate: true.

Also there is a spelling error (it is immeditate: true, right now), so that might also be a problem.

Sidenote: vueuse useVModel might also be of help for the value syncing here).

If you use `watchEffect` here you can save the definition of what to watch (vue does that automatically). Also it runs initially by default, so no need for `immediate: true`. Also there is a spelling error (it is `immeditate: true,` right now), so that might also be a problem. Sidenote: [vueuse useVModel](https://vueuse.org/core/usevmodel/) might also be of help for the value syncing here).

konrad commented

2021-12-07 19:46:50 +00:00

Or use a ref. But then you need to assign to list.value

I thought ref should be used preferrably for primitive types, not objects?

Also there is a spelling error (it is immeditate: true, right now), so that might also be a problem.

wow now I feel a bit dumb 😄 It seems to work if I correct it.

> Or use a ref. But then you need to assign to list.value I thought `ref` should be used preferrably for primitive types, not objects? > Also there is a spelling error (it is immeditate: true, right now), so that might also be a problem. wow now I feel a bit dumb 😄 It seems to work if I correct it.

konrad commented

2021-12-07 19:55:01 +00:00

watchEffect seems to work great, switched it to this and Object.assign.

`watchEffect` seems to work great, switched it to this and `Object.assign`.

konrad marked this conversation as resolved

src/components/tasks/partials/listSearch.vue Outdated

						
				@ -66,2 +57,2 @@

						},

					},

				function select(l) {

					list = l

dpschen commented

2021-12-06 13:03:11 +00:00

Use Object.assign

Use `Object.assign`

konrad marked this conversation as resolved

src/components/tasks/partials/listSearch.vue Outdated

						
				@ -68,0 +62,4 @@

				function namespace(namespaceId) {

					const namespace = store.getters['namespaces/getNamespaceById'](namespaceId)

					if (namespace !== null) {

dpschen commented

2021-12-06 13:05:58 +00:00


return namespace !== null
    ? namespace.title
    : t('list.shared')

```js return namespace !== null ? namespace.title : t('list.shared') ```

konrad commented

2021-12-07 19:42:45 +00:00

Done.

konrad marked this conversation as resolved

dpschen commented

2021-12-06 13:15:31 +00:00

Maybe related:

I also realized that there is no default list initially.

So even if it returns 0 will this work?

Maybe related: I also realized that there is no default list initially. So even if it returns `0` will this work?

konrad added 3 commits 2021-12-07 19:42:43 +00:00

chore: fix padding duplication b1824e752c

chore: don't overly use t instead of $t 51d90e9452

chore: simplify return

continuous-integration/drone/pr Build is failing

Details

c9c139913e

konrad added 1 commit 2021-12-07 19:45:02 +00:00

fix: usage of immediate

continuous-integration/drone/pr Build is passing

Details

c441283909

konrad added 1 commit 2021-12-07 19:54:30 +00:00

fix: use watchEffect and prevent loosing reactivity

continuous-integration/drone/pr Build is passing

Details

8c4aeec53f

konrad requested review from dpschen 2021-12-07 19:55:13 +00:00

konrad commented

2021-12-07 19:57:02 +00:00

I also realized that there is no default list initially.

So even if it returns 0 will this work?

It wont work in that case. The default list is 0 because that's the nil value of an int in Go. Setting it to 0 matches that behaviour but is not ideal.

> I also realized that there is no default list initially. > > So even if it returns `0` will this work? It wont work in that case. The default list is 0 because that's the nil value of an int in Go. Setting it to 0 matches that behaviour but is not ideal.

konrad added 1 commit 2021-12-07 20:04:07 +00:00

chore: add const for default list id

continuous-integration/drone/pr Build is failing

Details

49e44901b2

dpschen requested changes 2021-12-07 21:39:42 +00:00

src/components/tasks/partials/listSearch.vue Outdated

						
				@ -35,0 +26,4 @@

				const store = useStore()

				const {t} = useI18n()

				let list = reactive(new ListModel())

dpschen commented

2021-12-06 15:07:36 +00:00

A reactive has to be a const. If you reassign it, it wouldn't be a reactive anymore. Same goes for refs.

If you think of refs as reactives with just one exposed prop that is named value it's easier to understand.
Also good to know is that vues reactivity is now based on Proxies. When you know this it makes sense that reactivity works only on the objects props (.value / the props of a reactive) and you cannot simply replace the proxy.

(if you knew all of that, tell me and I'll stop explaing)

A reactive has to be a `const`. If you reassign it, it wouldn't be a reactive anymore. Same goes for refs. If you think of refs as reactives with just one exposed prop that is named value it's easier to understand. Also good to know is that vues reactivity is now based on [Proxies](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy). When you know this it makes sense that reactivity works only on the objects props (`.value` / the props of a reactive) and you cannot simply replace the proxy. (if you knew all of that, tell me and I'll stop explaing)

konrad commented

2021-12-11 15:11:44 +00:00

Thanks for the explanation, that makes a lot of sense.

(if you knew all of that, tell me and I'll stop explaing)

No worries! I kind of know bits and pieces of it, not the whole thing :)

Thanks for the explanation, that makes a lot of sense. > (if you knew all of that, tell me and I'll stop explaing) No worries! I kind of know bits and pieces of it, not the whole thing :)

👍 1

konrad marked this conversation as resolved

src/components/tasks/partials/listSearch.vue Outdated

						
				@ -51,2 +47,2 @@

							this.foundLists = this.$store.getters['lists/searchList'](query)

						},

				)

				list = props.modelValue

dpschen commented

2021-12-06 15:08:41 +00:00

I overlooked this when I first checked this.
With the watcher / watchEffect this shouldn't be necessary.

I overlooked this when I first checked this. With the watcher / watchEffect this shouldn't be necessary.

konrad commented

2021-12-11 15:12:44 +00:00

Yes, this works.

konrad marked this conversation as resolved

src/components/tasks/partials/listSearch.vue Outdated

						
				@ -67,1 +57,3 @@

					},

				function select(l) {

					list = l

					emit('selected', list)

dpschen commented

2021-12-06 15:24:02 +00:00

I think we should remove the selected and only use update:modelValue to reduce complexity, since they are doing the same.

I think we should remove the `selected` and only use `update:modelValue` to reduce complexity, since they are doing the same.

konrad commented

2021-12-11 15:15:04 +00:00

I didn't know you could listen to update:modelValue explicitly. Removed selected.

I didn't know you could listen to `update:modelValue` explicitly. Removed `selected`.

👍 1

dpschen commented

2021-12-12 13:10:36 +00:00

AFAIK it really is just a convention, other than that a regular event.

konrad marked this conversation as resolved

dpschen reviewed 2021-12-07 21:51:08 +00:00

src/views/user/settings/General.vue Outdated

						
				@ -205,3 +214,3 @@

							saveLanguage(this.language)

							setQuickAddMagicMode(this.quickAddMagicMode)

							this.settings.defaultListId = this.defaultList ? this.defaultList.id : 0

							this.settings.defaultListId = this.defaultList ? this.defaultList.id : DEFAULT_LIST_ID

dpschen commented

2021-12-07 21:51:08 +00:00

Since this is before sending to the server we should just create a new object here (and not modify the settings directly). Something like (super untested):

const settings = {
	...this.settings, // maybe needs copy
    defaultListId ? this.defaultList.id : DEFAULT_LIST_ID,
}

const newSettings = await this.userSettingsService.update(settings)


this.$store.commit('auth/setUserSettings', newSettings)

Since this is before sending to the server we should just create a new object here (and not modify the settings directly). Something like (super untested): ```js const settings = { ...this.settings, // maybe needs copy defaultListId ? this.defaultList.id : DEFAULT_LIST_ID, } const newSettings = await this.userSettingsService.update(settings) this.$store.commit('auth/setUserSettings', newSettings) ```

konrad commented

2021-12-11 15:20:09 +00:00

Changed it to do just that. The api does not return the new settings though, only a success message - I'm committing the settings object built previously.

konrad marked this conversation as resolved

konrad added 1 commit 2021-12-11 15:12:05 +00:00

chore: make list const

continuous-integration/drone/pr Build was killed

Details

85dfed5bac

konrad added 1 commit 2021-12-11 15:14:38 +00:00

chore: remove selected event

continuous-integration/drone/pr Build was killed

Details

51d8405353

konrad added 1 commit 2021-12-11 15:19:16 +00:00

chore: create new settings object before saving

continuous-integration/drone/pr Build is passing

Details

0080ad2077

dpschen requested changes 2021-12-12 13:29:00 +00:00

src/components/input/multiselect.vue

						
				@ -478,3 +478,3 @@

								color: transparent;

								transition: color $transition;

								padding: 0 .5rem;

								padding-left: .5rem;

dpschen commented

2021-12-12 13:12:44 +00:00

Not sure if by intend: you removed the padding-right.

Not sure if by intend: you removed the `padding-right`.

konrad commented

2021-12-12 14:02:10 +00:00

Yes that was intended because the text on the right "press enter or click to select" already has padding.

👍 1

konrad marked this conversation as resolved

src/components/tasks/partials/listSearch.vue Outdated

						
				@ -18,3 +18,2 @@

				<script>

				import ListModel from '../../../models/list'

				<script setup>

dpschen commented

2021-12-12 13:13:30 +00:00

Use ts

konrad commented

2021-12-12 14:07:25 +00:00

I don't know why but I can't seem to use the component anymore when I add it:

 SyntaxError: import not found: default client.ts:33:12
    warnFailedFetch client.ts:33
    fetchUpdate client.ts:334
    fetchUpdate client.ts:321
    handleMessage client.ts:72
    handleMessage client.ts:70
    <anonymous> client.ts:44
    (Async: EventListener.handleEvent)
    <anonymous> client.ts:43
15:06:39.577
[hmr] Failed to reload /src/components/tasks/partials/listSearch.vue. This could be due to syntax errors or importing non-existent modules. (see errors above)

I don't know why but I can't seem to use the component anymore when I add it: ``` SyntaxError: import not found: default client.ts:33:12 warnFailedFetch client.ts:33 fetchUpdate client.ts:334 fetchUpdate client.ts:321 handleMessage client.ts:72 handleMessage client.ts:70 <anonymous> client.ts:44 (Async: EventListener.handleEvent) <anonymous> client.ts:43 15:06:39.577 [hmr] Failed to reload /src/components/tasks/partials/listSearch.vue. This could be due to syntax errors or importing non-existent modules. (see errors above) ```

konrad commented

2021-12-12 19:55:56 +00:00

I don't know why it did not work previously, seems to work now.

konrad marked this conversation as resolved

src/components/tasks/partials/listSearch.vue

						
				@ -48,0 +27,4 @@

				const {t} = useI18n()

				const list = reactive(new ListModel())

				let foundLists = ref([])

dpschen commented

2021-12-12 13:13:07 +00:00

Ref foundLists should be a const.

Ref `foundLists` should be a const.

dpschen commented

2021-12-12 13:15:04 +00:00

Move foundLists definition next to where it is used (the findLists function).
=> Group by function; More vue composition way of doing things.

Move `foundLists` definition next to where it is used (the findLists function). => Group by function; More vue composition way of doing things.

👍 1

konrad commented

2021-12-12 14:08:47 +00:00

Done.

konrad marked this conversation as resolved

src/components/tasks/partials/listSearch.vue Outdated

						
				@ -48,0 +30,4 @@

				let foundLists = ref([])

				const props = defineProps({

					modelValue: {

						required: false,

dpschen commented

2021-12-12 13:13:21 +00:00

Add type

konrad commented

2021-12-12 14:09:17 +00:00

I had to use a custom validation because the ListModel is not really a type.

I had to use a custom validation because the `ListModel` is not really a type.

konrad marked this conversation as resolved

src/components/tasks/partials/listSearch.vue Outdated

						
				@ -68,0 +47,4 @@

				}

				function select(l) {

					Object.assign(list, l)

dpschen commented

2021-12-12 13:18:03 +00:00

Is the Object.assign here even needed?

Asking because:
Since you emit the new value in the next line it should already be changed via the update of the modelValue in the watchEffect. In the sense of events up / props down.

Is the `Object.assign` here even needed? Asking because: Since you emit the new value in the next line it should already be changed via the update of the modelValue in the watchEffect. In the sense of events up / props down.

dpschen commented

2021-12-12 13:19:30 +00:00

Okay I just saw that modelValue is not required and also not always set (e.g. in the TaskDetailView).
Then of course it's still needed.

Okay I just saw that modelValue is not required and also not always set (e.g. in the TaskDetailView). Then of course it's still needed.

konrad marked this conversation as resolved

src/views/user/settings/General.vue

						
				@ -211,2 +217,3 @@

							const settings = {

								...this.settings,

							})

								defaultListId: this.defaultList ? this.defaultList.id : DEFAULT_LIST_ID,

dpschen commented

2021-12-12 13:22:54 +00:00

I think I know now what seemed of for me:
defaultListId should be a computed.
This way the logic this.defaultList ? this.defaultList.id : DEFAULT_LIST_ID does not have to be repeated here and in the defaultList setter.

I think I know now what seemed of for me: defaultListId should be a computed. This way the logic `this.defaultList ? this.defaultList.id : DEFAULT_LIST_ID` does not have to be repeated here and in the defaultList setter.

konrad commented

2021-12-12 14:11:51 +00:00

We actually don't need this at all due to the setter. I've removed it.

🎉 1

dpschen commented

2021-12-12 14:36:23 +00:00

Didn't realize this aswell. Nice!

🎉 1

konrad marked this conversation as resolved

konrad added 1 commit 2021-12-12 14:08:38 +00:00

chore: add validation

continuous-integration/drone/pr Build is passing