feat: namespace settings archive script setup #2357

Merged
dpschen merged 1 commits from dpschen/frontend:feature/namespace-settings-archive-script-setup into main 2022-09-15 20:46:28 +00:00
Member
No description provided.
Member

Hi dpschen!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://2357-feature-namespace-settings-archi--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 dpschen! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://2357-feature-namespace-settings-archi--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.
dpschen changed title from WIP: feat: namespace settings archive script setup to feat: namespace settings archive script setup 2022-09-13 16:00:36 +00:00
dpschen requested review from konrad 2022-09-13 16:00:42 +00:00
konrad was assigned by dpschen 2022-09-13 16:00:46 +00:00
konrad requested changes 2022-09-13 19:01:02 +00:00
konrad left a comment
Owner

Looks like the title is missing now:

image

vs on try:

image

Looks like the title is missing now: ![image](/attachments/14376098-63b5-4e60-8dbb-e14efe50eeb1) vs on try: ![image](/attachments/f0d0e079-3950-4512-b0ef-3eea9a1f656a)
189 KiB
156 KiB
dpschen force-pushed feature/namespace-settings-archive-script-setup from 5399abb5cc to 49df1324f8 2022-09-15 09:14:53 +00:00 Compare
Author
Member

This should work now.
The namespaces were sometimes not loaded thus the title wasn't ready to be read when the component mounted. Seemed like a race condition.

For the future it would be better if we could put the loadSomeRessource mechanisms in the store so that all references to that ressource in the frontend are automatically updated. Right now I copied over the code from the EditNamespace component which loads the namespace again just for the modal.

I didn't find out why this race condition appears now.
When I put in a small setTimout the namespaces would be there but this seems super error-prone.

This should work now. The namespaces were sometimes not loaded thus the title wasn't ready to be read when the component mounted. Seemed like a race condition. For the future it would be better if we could put the loadSomeRessource mechanisms in the store so that all references to that ressource in the frontend are automatically updated. Right now I copied over the code from the EditNamespace component which loads the namespace again just for the modal. I didn't find out why this race condition appears now. When I put in a small setTimout the namespaces would be there but this seems super error-prone.
Author
Member

This includes also an update on the useTitle composable which is now a slim wrapper around the vue-use composable with the same name that adds the Vikunja suffixes :)

This includes also an update on the `useTitle` composable which is now a slim wrapper around the vue-use composable with the same name that adds the Vikunja suffixes :)
Owner

Looks like the title still doesn't work :/

image

Looks like the title still doesn't work :/ ![image](/attachments/4373670f-6988-48e4-ba52-3c9472e3915b)
konrad reviewed 2022-09-15 10:15:00 +00:00
@ -54,0 +54,4 @@
watch(
() => props.namespaceId,
async () => {
Object.assign(namespace, store.getters['namespaces/getNamespaceById'](props.namespaceId))

Shouldn't this work? The namespace is always in store since it is used in the menu. We'll only need the title so it should be enough to get it from store.

Shouldn't this work? The namespace is always in store since it is used in the menu. We'll only need the title so it should be enough to get it from store.
Author
Member

As written above:

I didn't find out why this race condition appears now.
When I put in a small setTimout the namespaces would be there but this seems super error-prone.

As written above: > I didn't find out why this race condition appears now. When I put in a small setTimout the namespaces would be there but this seems super error-prone.
@ -54,0 +57,4 @@
Object.assign(namespace, store.getters['namespaces/getNamespaceById'](props.namespaceId))
// FIXME: ressouce should be loaded in store
// Object.assign(namespace, await namespaceService.get({id: props.namespaceId}))

Can you remove this commented code? Or uncomment it?

Can you remove this commented code? Or uncomment it?
Author
Member

Fixed

Fixed
dpschen marked this conversation as resolved
dpschen was assigned by konrad 2022-09-15 11:36:09 +00:00
konrad removed their assignment 2022-09-15 11:36:13 +00:00
dpschen force-pushed feature/namespace-settings-archive-script-setup from 49df1324f8 to 7860d233d9 2022-09-15 17:49:28 +00:00 Compare
konrad approved these changes 2022-09-15 20:38:47 +00:00
konrad left a comment
Owner

Looks good.

Looks good.
dpschen merged commit ad6b335d41 into main 2022-09-15 20:46:28 +00:00
dpschen deleted branch feature/namespace-settings-archive-script-setup 2022-09-15 20:46:28 +00:00
This repo is archived. You cannot comment on pull requests.
No description provided.