feature/feat-useList-composable #2589

Closed
dpschen wants to merge 7 commits from dpschen/frontend:feature/feat-useList-composable into main
Member
No description provided.
dpschen force-pushed feature/feat-useList-composable from e75f00744d to b715cf4d70 2022-11-09 11:28:26 +00:00 Compare
dpschen force-pushed feature/feat-useList-composable from b715cf4d70 to 54564271fe 2022-11-18 15:59:30 +00:00 Compare
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://2589-feature-feat-uselist-composable--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://2589-feature-feat-uselist-composable--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 added 2 commits 2022-11-18 16:54:30 +00:00
dpschen changed title from WIP: feature/feat-useList-composable to feature/feat-useList-composable 2022-11-18 17:04:30 +00:00
dpschen requested review from konrad 2022-11-18 17:04:38 +00:00
konrad was assigned by dpschen 2022-11-18 17:04:43 +00:00
dpschen added the
changes requested
area/internal-code
labels 2022-11-18 17:04:52 +00:00
dpschen reviewed 2022-11-19 14:09:35 +00:00
@ -0,0 +1,195 @@
<template>
Author
Member

I still have to delete this.

I still have to delete this.
Author
Member

Done

Done
dpschen marked this conversation as resolved
@ -169,7 +169,6 @@
"title": "List Title",
"color": "Color",
"lists": "Lists",
"list": "List",
Author
Member

this entry is beeing overwritten below

this entry is beeing overwritten below
konrad marked this conversation as resolved
@ -328,3 +297,3 @@
return response.data.map(entry => this.modelGetAllFactory(entry))
}
return this.modelGetAllFactory(response.data)
return [this.modelGetAllFactory(response.data)]
Author
Member

this seemed wrong. when wouldn't you receive an array here?

this seemed wrong. when wouldn't you receive an array here?

I think there really is no case where no array is returned.

I think there really is no case where no array is returned.
Author
Member

Maybe it would be better to throw instead. Because this fucks up the return types. That's why I added the brackets.

Maybe it would be better to throw instead. Because this fucks up the return types. That's why I added the brackets.
Author
Member

Fixed

Fixed
konrad marked this conversation as resolved
@ -45,0 +47,4 @@
return
}
const cancel = this.setLoading()
Author
Member

unsure: maybe I should remove the setLoading here since the list shouldn't indicate a loading just for the background?

unsure: maybe I should remove the setLoading here since the list shouldn't indicate a loading just for the background?

I think that would make sense.

I think that would make sense.
Author
Member

Fixed

Fixed
dpschen marked this conversation as resolved
@ -136,8 +94,6 @@ export const useBaseStore = defineStore('base', () => {
loading: readonly(loading),
ready: readonly(ready),
currentList: readonly(currentList),
background: readonly(background),
Author
Member

I might need to readd this. I didn't realize that the info if a background is loaded is necessary.

I might need to readd this. I didn't realize that the info _if_ a background is loaded is necessary.
@ -95,2 +104,4 @@
}
async function loadList(listId: List['id']) {
const cancel = setModuleLoading(this, setIsLoading)
Author
Member

remove 'this'

remove 'this'
Author
Member

Was already fixed

Was already fixed
dpschen marked this conversation as resolved
@ -162,0 +216,4 @@
namespaceStore.addListToNamespace(duplicate.list)
setList(duplicate.list)
success({message: i18n.global.t('list.duplicate.success')})
router.push({name: 'list.index', params: {listId: duplicate.list.id}})
Author
Member

in case of the modal, where the modal has a view we should replace the view instead. maybe always?

in case of the modal, where the modal has a view we should replace the view instead. maybe always?

Doesn't the view in list.index do that?

Doesn't the view in `list.index` do that?
Author
Member

Yes it does. But we still use router.push(). What I meant was using router.replace() instead, because you wouldn't want to go back to the route of the dialog.

Yes it does. But we still use `router.push()`. What I meant was using `router.replace()` instead, because you wouldn't want to go back to the route of the dialog.

For routes like the task detail popup we should have the option to go back to the route with a popup. Maybe for settings as well.

For routes like the task detail popup we should have the option to go back to the route with a popup. Maybe for settings as well.
Author
Member

Yes, maybe this should be configurable via a prop

Yes, maybe this should be configurable via a prop
Author
Member

Keeping this as it is for now. This is something that should be tackled in the modal or even in the dialog branch later.

Keeping this as it is for now. This is something that should be tackled in the modal or even in the dialog branch later.
dpschen marked this conversation as resolved
@ -162,0 +222,4 @@
const backgroundService = shallowReactive(new BackgroundUnsplashService())
async function setUnsplashBackground(listId: IList['id'], backgroundId: IBackgroundImage['id']) {
// Don't set a background if we're in the process of setting one
Author
Member

why not?

why not?

I think this was to prevent multiple clicks quickly after each other. The better solution would of course be disabling the button.

I think this was to prevent multiple clicks quickly after each other. The better solution would of course be disabling the button.
Author
Member

Okay, will do that instead.

Okay, will do that instead.
@ -162,0 +227,4 @@
return
}
const list: IList = await backgroundService.update({
Author
Member

fix: type should be returned by update.

fix: type should be returned by update.
Author
Member

Fixed

Fixed
dpschen marked this conversation as resolved
@ -211,0 +457,4 @@
watch(
() => [listVal.value.id, listVal.value.backgroundBlurHash] as [IList['id'], IList['backgroundBlurHash']],
async ([listId, blurHash], oldValue) => {
if (backgroundLoading.value) {
Author
Member

I copied this from the old code, but I think that we shouldn't return in that case

I copied this from the old code, but I think that we shouldn't return in that case
@ -211,0 +475,4 @@
await listStore.loadListBackground(
listVal.value,
(value) => {
blurHashUrl.value = value
Author
Member

if the list or background has changed in between we shouldn't set this.

if the list or background has changed in between we shouldn't set this.

When could that be the case?

When could that be the case?
Author
Member

If you click on one list that has a background and then on another that has none. The promise would still finish and then set the (wrong) background.

If you click on one list that has a background and then on another that has none. The promise would still finish and then set the (wrong) background.

Makes sense!

Makes sense!
dpschen added 1 commit 2022-11-21 12:30:10 +00:00
continuous-integration/drone/pr Build is passing Details
fad87f12e0
fix: setFavorite
dpschen force-pushed feature/feat-useList-composable from fad87f12e0 to f80be88cb3 2022-12-09 14:55:12 +00:00 Compare
dpschen force-pushed feature/feat-useList-composable from f80be88cb3 to 86184a4f9f 2022-12-09 15:25:37 +00:00 Compare
Owner

Is this done?

Is this done?
dpschen force-pushed feature/feat-useList-composable from 86184a4f9f to 7e6aa0a9f4 2023-02-11 11:40:44 +00:00 Compare
dpschen force-pushed feature/feat-useList-composable from 7e6aa0a9f4 to 531b0baf02 2023-02-11 11:56:38 +00:00 Compare
dpschen force-pushed feature/feat-useList-composable from 531b0baf02 to 5fbd4c80cb 2023-02-11 12:35:30 +00:00 Compare
Owner

Closing this as it appears stale.

Please rebase and reopen if you think this is still relevant.

Closing this as it appears stale. Please rebase and reopen if you think this is still relevant.
konrad closed this pull request 2023-10-22 17:36:06 +00:00
This repo is archived. You cannot comment on pull requests.
No description provided.