feat: implement modals with vue router 4 #816

Merged
konrad merged 62 commits from dpschen/frontend:feature/vue3-modals-with-router-4 into main 2022-02-05 16:49:04 +00:00
Member

This is a work in progress (!) of the implementation of the modals with the new possibilities of vue router 3.
It's not working yet, I just wanted to share this early to be open for feedback.

Using this functionality would also allow us to use the same routes for all the modal views even when different "background views" are loaded.

See: https://github.com/vuejs/vue-router/issues/703#issuecomment-865066913 for a better explanation how this should work.
and the linked example implementation: https://github.com/vuejs/vue-router-next/blob/master/e2e/modal/index.ts

This is a work in progress (!) of the implementation of the modals with the new possibilities of vue router 3. It's not working yet, I just wanted to share this early to be open for feedback. Using this functionality would also allow us to use the same routes for all the modal views even when different "background views" are loaded. See: https://github.com/vuejs/vue-router/issues/703#issuecomment-865066913 for a better explanation how this should work. and the linked example implementation: https://github.com/vuejs/vue-router-next/blob/master/e2e/modal/index.ts
dpschen added the
help wanted
question
labels 2021-10-03 13:58:16 +00:00
Owner

Using this functionality would also allow us to use the same routes for all the modal views even when different "background views" are loaded.

That sounds really awesome.

> Using this functionality would also allow us to use the same routes for all the modal views even when different "background views" are loaded. That sounds really awesome.
konrad reviewed 2021-10-03 18:53:58 +00:00
@ -23,2 +23,3 @@
<router-view/>
<router-view :route="routeWithModal"/>
<!-- TODO: is this still used? -->

I don't think it is, we should make sure the modal views keep their transition though. Might make sense to include that in the modal component itself?

I don't think it is, we should make sure the modal views keep their transition though. Might make sense to include that in the modal component itself?
Author
Member

Either that or put the transition inside something like a provider component. In that we could use the new teleport component. I was always using portal-vue in vue 2 for this kind of stuff.

Either that or put the transition inside something like a provider component. In that we could use the [new teleport component](https://v3.vuejs.org/api/built-in-components.html#teleport). I was always using [portal-vue](https://github.com/LinusBorg/portal-vue) in vue 2 for this kind of stuff.

I think using the teleport component allows for a cleaner solution since there are situations where you want a transition handled by the route and others where you want to have it handled by the outer component (like delete modals).

I think using the teleport component allows for a cleaner solution since there are situations where you want a transition handled by the route and others where you want to have it handled by the outer component (like delete modals).
dpschen marked this conversation as resolved
Author
Member

Sidenote: stumbled upon this related thread...

Sidenote: stumbled upon [this related thread...](https://mobile.twitter.com/rsms/status/1435636310371291137)
konrad closed this pull request 2021-10-17 21:21:05 +00:00
konrad reopened this pull request 2021-10-17 21:27:41 +00:00
dpschen force-pushed feature/vue3-modals-with-router-4 from 78834283d8 to 3fb1e2ea2b 2021-10-25 19:37:32 +00:00 Compare
dpschen self-assigned this 2021-11-01 00:41:05 +00:00
dpschen force-pushed feature/vue3-modals-with-router-4 from 3f1988d723 to 8b43bfe224 2021-11-01 09:33:35 +00:00 Compare
dpschen force-pushed feature/vue3-modals-with-router-4 from 8b43bfe224 to 6830d8e0c1 2021-11-01 17:20:25 +00:00 Compare
Author
Member

@konrad: the tests might not look like it but this is now mostly working.
I'm trying to figure out the missing details. Might need some info from you for some of the stuff.

We will get some merge conflicts. Especially with the auth branch. Originally I had some auth logic included in this branch but separated it when I saw that you changed the auth. The reason I had for already including some auth was that I use the route meta fields for defining what is opened in a popover and what not.

This branch also makes taskList finally a composable so that no mixins are left apart from the small global helpers defined in main.ts

@konrad: the tests might not look like it but this is now mostly working. I'm trying to figure out the missing details. Might need some info from you for some of the stuff. We will get some merge conflicts. Especially with the auth branch. Originally I had some auth logic included in this branch but separated it when I saw that you changed the auth. The reason I had for already including some auth was that I use the route `meta` fields for defining what is opened in a popover and what not. This branch also makes taskList finally a composable so that no mixins are left apart from the small global helpers defined in `main.ts`
dpschen force-pushed feature/vue3-modals-with-router-4 from 8eb2e3f281 to 6f4fea6ffc 2021-11-08 15:12:20 +00:00 Compare
Owner

We will get some merge conflicts. Especially with the auth branch.

You mean #926 ?

> We will get some merge conflicts. Especially with the auth branch. You mean https://kolaente.dev/vikunja/frontend/pulls/926 ?
Author
Member

We will get some merge conflicts. Especially with the auth branch.

You mean #926 ?

Yes and with #899

> > We will get some merge conflicts. Especially with the auth branch. > > You mean https://kolaente.dev/vikunja/frontend/pulls/926 ? Yes and with https://kolaente.dev/vikunja/frontend/pulls/899
dpschen force-pushed feature/vue3-modals-with-router-4 from 6f4fea6ffc to 3f1cecca7b 2021-11-14 17:46:15 +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://816-featurevue3-modals-with-router-4--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://816-featurevue3-modals-with-router-4--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: implement modals with vue router 4 to feat: implement modals with vue router 4 2021-11-14 21:03:52 +00:00
Author
Member

@konrad: can you help me fix the remaining issues here =)

@konrad: can you help me fix the remaining issues here =)
Owner

Sure! Do you have a rough list of things that need fixing? Or just replacing every modal view to use the new syntax?

Sure! Do you have a rough list of things that need fixing? Or just replacing every modal view to use the new syntax?
Author
Member

I thought it's mainly fixing the tests now.
Changing the other modals should be a different task.

I thought it's mainly fixing the tests now. Changing the other modals should be a different task.
dpschen force-pushed feature/vue3-modals-with-router-4 from 97133107d1 to 68b6fb8f38 2021-11-17 17:15:48 +00:00 Compare
dpschen force-pushed feature/vue3-modals-with-router-4 from ec74b1e9a0 to c63991b3a8 2021-11-27 13:19:37 +00:00 Compare
Author
Member

@konrad:

Tests that I fail to fix and have no clue where the issue is coming from:

  • sharing/linkShare.spec.js
  • task/task.spec.js => Can remove a label from a task
  • user/login.spec.js
  • user/registration.spec.js

The rest of the failing tests could be solved via the getSettled workaround.

@konrad: Tests that I fail to fix and have no clue where the issue is coming from: - [x] ~~sharing/linkShare.spec.js~~ - [x] task/task.spec.js => Can remove a label from a task - [x] ~~user/login.spec.js~~ - [x] ~~user/registration.spec.js~~ The rest of the failing tests could be solved via the `getSettled` workaround.
dpschen force-pushed feature/vue3-modals-with-router-4 from 314b2f5935 to 54aaa4c836 2021-11-28 16:30:46 +00:00 Compare
dpschen force-pushed feature/vue3-modals-with-router-4 from 54aaa4c836 to 1296f1c748 2021-11-29 15:54:33 +00:00 Compare
dpschen force-pushed feature/vue3-modals-with-router-4 from 1296f1c748 to 99691cc149 2021-11-30 16:44:20 +00:00 Compare
dpschen force-pushed feature/vue3-modals-with-router-4 from 99691cc149 to 011207dfb5 2021-11-30 18:57:45 +00:00 Compare
dpschen force-pushed feature/vue3-modals-with-router-4 from 598362a859 to 7e4a57b13e 2021-11-30 20:11:21 +00:00 Compare
dpschen removed the
question
label 2021-11-30 20:13:50 +00:00
dpschen requested review from konrad 2021-11-30 20:13:53 +00:00
konrad was assigned by dpschen 2021-11-30 20:13:57 +00:00
dpschen removed their assignment 2021-11-30 20:14:03 +00:00
dpschen added
kind/feature
area/internal-code
and removed
help wanted
labels 2021-11-30 20:27:44 +00:00
konrad reviewed 2021-12-04 15:18:26 +00:00
cypress.json Outdated
@ -10,1 +10,3 @@
}
},
"testFiles": [
"list/list.spec.js",

Why is the list.spec.js seperated from the rest?

Why is the `list.spec.js` seperated from the rest?
Author
Member

Because I splitted the test from the different views list.spec.js was not the first test but the last after the views (because it's imported by abc order).
With this the tests in the file stay at the beginning.
Maybe better to prefix with numbers to maintain the order? Or just an understore for the general list tests to force the correct order?

Because I splitted the test from the different views list.spec.js was not the first test but the last after the views (because it's imported by abc order). With this the tests in the file stay at the beginning. Maybe better to prefix with numbers to maintain the order? Or just an understore for the general list tests to force the correct order?

The order of the tests should not matter. Each test should work the same whether it was executed with all other ones in one run or individually.
I think we can ignore the acutal order in which the tests are executed.

The order of the tests should not matter. Each test should work the same whether it was executed with all other ones in one run or individually. I think we can ignore the acutal order in which the tests are executed.
Author
Member

Done. Checking if the tests run through.
they should, but I remember that I had some problems, maybe that's solved)

Done. Checking if the tests run through. they should, but I remember that I had some problems, maybe that's solved)

Looks like there's a syntax error in the cypress config file now: https://drone.kolaente.de/vikunja/frontend/5026/1/7

Looks like there's a syntax error in the cypress config file now: https://drone.kolaente.de/vikunja/frontend/5026/1/7
Author
Member

Sometimes it's really cool, if you know JSON #fail
(was a line end comma)

Sometimes it's really cool, if you know JSON #fail (was a line end comma)
dpschen marked this conversation as resolved
@ -265,2 +265,4 @@
overflow-y: auto
}
@include modal-transition();

Doesn't repeating this create duplicate css everywhere? (even if very little)

Doesn't repeating this create duplicate css everywhere? (even if very little)
Author
Member

Yes it does for now. But it shouldn't stay:

By using a mixin I create a direct dependency. The reason for me doing this is, that I want to change this later when working on the Modal / Dialog feature.

Perceive it as a reminder =)

Yes it does for now. But it shouldn't stay: By using a mixin I create a direct dependency. The reason for me doing this is, that I want to change this later when working on the Modal / Dialog feature. Perceive it as a reminder =)
dpschen marked this conversation as resolved
@ -105,0 +107,4 @@
return {
name: 'task.detail',
params: { id: this.taskEditTask.id },
state: { backgroundView: this.$router.currentRoute.value.fullPath },

I'm not sure about the naming of this because there's views with a background. Maybe that can get confusing?

I'm not sure about the naming of this because there's views with a background. Maybe that can get confusing?
Author
Member

That's true. Do you have a suggestion?

Maybe something like backdropView in the sense of https://developer.mozilla.org/en-US/docs/Web/CSS/backdrop-filter

That's true. Do you have a suggestion? Maybe something like backdropView in the sense of https://developer.mozilla.org/en-US/docs/Web/CSS/backdrop-filter

I like that idea.

I like that idea.
Author
Member

Done

Done
dpschen marked this conversation as resolved
@ -1,102 +0,0 @@
import TaskCollectionService from '@/services/taskCollection'

Oh you got rid of the mixin, very nice!

Oh you got rid of the mixin, very nice!
dpschen marked this conversation as resolved
@ -0,0 +21,4 @@
const loading = computed(() => taskCollectionService.value.loading)
const totalPages = computed(() => taskCollectionService.value.totalPages)
const tasks = ref([])

Please indent.

Please indent.
Author
Member

Done

Done
dpschen marked this conversation as resolved
Owner

There's this error happening when opening a task from the list view:

image

There's this error happening when opening a task from the list view: ![image](/attachments/1c1e43b0-f1aa-4fc1-9210-4a58e5b8982c)
Owner

It looks like the list is reloaded every time I close an opened settings dialog or a task from the kanban board. The same is happening when I open a settings dialog from the menu for other lists than the one currently open. I think it should keep the state when opening modals to avoid unnessecary reloads.

It looks like the list is reloaded every time I close an opened settings dialog or a task from the kanban board. The same is happening when I open a settings dialog from the menu for other lists than the one currently open. I think it should keep the state when opening modals to avoid unnessecary reloads.
dpschen force-pushed feature/vue3-modals-with-router-4 from 7f97c34787 to 08aa701924 2021-12-04 16:17:01 +00:00 Compare
Author
Member

There's this error happening when opening a task from the list view:

image

Seems to be the same as https://sentry.io/organizations/vikunja/issues/2810598130/?project=6024480&query=is%3Aunresolved&statsPeriod=90d

> There's this error happening when opening a task from the list view: > > ![image](/attachments/1c1e43b0-f1aa-4fc1-9210-4a58e5b8982c) Seems to be the same as https://sentry.io/organizations/vikunja/issues/2810598130/?project=6024480&query=is%3Aunresolved&statsPeriod=90d
dpschen force-pushed feature/vue3-modals-with-router-4 from 08aa701924 to 7b4bb80ec3 2021-12-04 16:35:19 +00:00 Compare
Author
Member

It looks like the list is reloaded every time I close an opened settings dialog or a task from the kanban board. The same is happening when I open a settings dialog from the menu for other lists than the one currently open. I think it should keep the state when opening modals to avoid unnessecary reloads.

Might be that I fixed this:

I forgot a .value here: https://kolaente.dev/dpschen/frontend/src/branch/feature/vue3-modals-with-router-4/src/views/list/ListWrapper.vue#L127

> It looks like the list is reloaded every time I close an opened settings dialog or a task from the kanban board. The same is happening when I open a settings dialog from the menu for other lists than the one currently open. I think it should keep the state when opening modals to avoid unnessecary reloads. Might be that I fixed this: I forgot a `.value` here: https://kolaente.dev/dpschen/frontend/src/branch/feature/vue3-modals-with-router-4/src/views/list/ListWrapper.vue#L127
Owner

It looks like the list is reloaded every time I close an opened settings dialog or a task from the kanban board. The same is happening when I open a settings dialog from the menu for other lists than the one currently open. I think it should keep the state when opening modals to avoid unnessecary reloads.

Might be that I fixed this:

I forgot a .value here: https://kolaente.dev/dpschen/frontend/src/branch/feature/vue3-modals-with-router-4/src/views/list/ListWrapper.vue#L127

Nope, still happening :/

> > It looks like the list is reloaded every time I close an opened settings dialog or a task from the kanban board. The same is happening when I open a settings dialog from the menu for other lists than the one currently open. I think it should keep the state when opening modals to avoid unnessecary reloads. > > Might be that I fixed this: > > I forgot a `.value` here: https://kolaente.dev/dpschen/frontend/src/branch/feature/vue3-modals-with-router-4/src/views/list/ListWrapper.vue#L127 Nope, still happening :/
dpschen force-pushed feature/vue3-modals-with-router-4 from e922f3beed to cdffc71298 2021-12-07 17:56:28 +00:00 Compare
Owner

Nope, still happening :/

Looks like this works now.

Now I get a vuex store maniputlation warning whenever I modify a task on the kanban board.

When I open a task from the list or table view in the popup and modify it, the new details are not updated in the list after closing the popup.

> Nope, still happening :/ Looks like this works now. Now I get a vuex store maniputlation warning whenever I modify a task on the kanban board. When I open a task from the list or table view in the popup and modify it, the new details are not updated in the list after closing the popup.
Author
Member

Nope, still happening :/

Looks like this works now.

Now I get a vuex store maniputlation warning whenever I modify a task on the kanban board.

When I open a task from the list or table view in the popup and modify it, the new details are not updated in the list after closing the popup.

Yeah this pull request is so complex :/

I tried but I really couldn't find out how to implement the new modals without implementing the taskList as composable
And the latter brings along the complexity

> > Nope, still happening :/ > > Looks like this works now. > > Now I get a vuex store maniputlation warning whenever I modify a task on the kanban board. > > When I open a task from the list or table view in the popup and modify it, the new details are not updated in the list after closing the popup. Yeah this pull request is so complex :/ I tried but I really couldn't find out how to implement the new modals without implementing the taskList as composable And the latter brings along the complexity
Owner

Yeah this pull request is so complex :/

Then maybe we should do that in a follow-up PR?

I think we should at least try to fix the vuex store manipulation warning because that kinda breaks the kanban board.

> Yeah this pull request is so complex :/ Then maybe we should do that in a follow-up PR? I think we should at least try to fix the vuex store manipulation warning because that kinda breaks the kanban board.
Author
Member

Yeah this pull request is so complex :/

Then maybe we should do that in a follow-up PR?

I think we should at least try to fix the vuex store manipulation warning because that kinda breaks the kanban board.

Absolutely! I'll check this.

> > Yeah this pull request is so complex :/ > > Then maybe we should do that in a follow-up PR? > > I think we should at least try to fix the vuex store manipulation warning because that kinda breaks the kanban board. Absolutely! I'll check this.
Owner

What happens when you enter the task url in the browser's address bar? Since everything is a popup now, what view will be opened behind it?

What happens when you enter the task url in the browser's address bar? Since everything is a popup now, what view will be opened behind it?
Author
Member

What happens when you enter the task url in the browser's address bar? Since everything is a popup now, what view will be opened behind it?

The list in which the task is defined =)

> What happens when you enter the task url in the browser's address bar? Since everything is a popup now, what view will be opened behind it? The list in which the task is defined =)
Owner

@dpschen and in what view? (list, Gantt, Kanban etc.) There could be cases where this is not saved in local storage just yet.

@dpschen and in what view? (list, Gantt, Kanban etc.) There could be cases where this is not saved in local storage just yet.
dpschen reviewed 2021-12-22 15:25:03 +00:00
@ -0,0 +17,4 @@
* This mixin provides a base set of methods and properties to get tasks on a list.
*/
export function createTaskList(initTasks) {
const taskCollectionService = ref(new TaskCollectionService())
Author
Member

Maybe shallowReactive fits here better?

Maybe shallowReactive fits here better?
dpschen marked this conversation as resolved
@ -0,0 +38,4 @@
) {
// Because this function is triggered every time on topNavigation, we're putting a condition here to only load it when we actually want to show tasks
// FIXME: This is a bit hacky -> Cleanup.
Author
Member

TODO: check if this works

TODO: check if this works
@ -504,0 +321,4 @@
return {
name: router.hasRoute(savedListView)
? savedListView
: 'list.list',
Author
Member

@konrad: is defined here

@konrad: is defined here

Ah okay that makes sense.

Ah okay that makes sense.
dpschen marked this conversation as resolved
@ -149,0 +89,4 @@
// switched to it. This ensures updates done to tasks in the gantt or list views are consistently
// shown in all views while preventing reloads when closing a task popup.
// We don't do this for the table view because that does not change tasks.
// FIXME: remove this
Author
Member

Remove this

Remove this
dpschen force-pushed feature/vue3-modals-with-router-4 from f8d57188e7 to 1a5032d8c6 2021-12-31 01:04:43 +00:00 Compare
dpschen force-pushed feature/vue3-modals-with-router-4 from 1a5032d8c6 to 6b59d2a9c9 2021-12-31 16:38:21 +00:00 Compare
Author
Member
- [ ] Add [scroll-gutter](https://t.co/0DCFbMnQ57) to background when modal is open
dpschen force-pushed feature/vue3-modals-with-router-4 from 6b59d2a9c9 to 43c935ca41 2022-01-04 19:55:26 +00:00 Compare
dpschen force-pushed feature/vue3-modals-with-router-4 from 43c935ca41 to e6e8a98514 2022-01-04 21:03:08 +00:00 Compare
konrad reviewed 2022-01-08 12:16:09 +00:00
@ -1,4 +1,5 @@
<template>
<!-- FIXME: transition should not be included in the modal -->

Where should it be included instead?

Where should it be included instead?
Author
Member

Around the modal. So where it will get mounted.

Around the modal. So where it will get mounted.

Ah okay, makes sense.

Ah okay, makes sense.
dpschen marked this conversation as resolved
@ -340,2 +340,4 @@
width: calc(100% - 48px - 2rem);
}
@include modal-transition();

Why does this component need the modal-transition css if it is never opened as a modal?

Why does this component need the modal-transition css if it is never opened as a modal?
Author
Member

It does have a modal inside! See line 135.
By importing it here we remove the indirect dependency.

It does have a modal inside! See line 135. By importing it here we remove the indirect dependency.
dpschen marked this conversation as resolved
@ -39,3 +39,3 @@
name: 'description',
components: {
editor: AsyncEditor,
Editor,

Why do you want to load the editor directly instead of async? Are you loading the whole task component async?

Why do you want to load the editor directly instead of async? Are you loading the whole task component async?
Author
Member

I was probably going to far here – will undo.

I guess loading the editor async might even make sense with a new smaller package.

I was probably going to far here – will undo. I guess loading the editor async might even make sense with a new smaller package.
dpschen marked this conversation as resolved
@ -0,0 +1,185 @@
<template>

If this isn't accessible as a "view" it should not go in src/views/

If this isn't accessible as a "view" it should not go in `src/views/`
Author
Member

I see the views folder more defined as something like

components that work just with / in specific views

The ListWrapper component makes only sense together with the other list views – as of now. If this changes I might change my view.

I also like to put components that only work with the router in view.
All components in components should work in isolation aka only props needed – no route.

Does this work for you?

I see the views folder more defined as something like > components that work just with / in specific views The `ListWrapper` component makes only sense together with the other list views – as of now. If this changes I might change my view. I also like to put components that only work with the router in view. All components in `components` should work in isolation aka only props needed – no route. Does this work for you?

Okay that explanation makes sense. I think we can keep it that way.

Okay that explanation makes sense. I think we can keep it that way.
konrad marked this conversation as resolved
@ -15,3 +6,1 @@
:userIsAdmin="userIsAdmin"
shareType="team"
type="namespace"/>
<template v-if="namespace">

Isn't the whole component empty when no namespace is present?

Isn't the whole component empty when no namespace is present?
Author
Member

Yes. As far as I remember this was a typescript issue.
Would probably be good to validate the namespace before entering though. Maybe something for a follow up?

Yes. As far as I remember this was a typescript issue. Would probably be good to validate the namespace before entering though. Maybe something for a follow up?

Yeah I think that could work for a follow-up. Added it to the list (my comment below this one).

Yeah I think that could work for a follow-up. Added it to the list (my comment below this one).
Owner

Here's what I think we should do in order to get this merged (other than the review comments):

  • Error s.available is not a function when opening the keyboard shortcuts modal
  • When opening a task in the popup from list or table view and changing task attributes (like the title) the list view still has the old details in it after closing the popup. Since this won't be an easy change, I'm fine with doing that in a follow-up PR but then we should revert to the old behaviour where it opens the task in a full page instead of a modal for the list and table views.

Here's what we should do related to this PR, but things that can happen in follow-up PRs:

  • Some style issues, probably caused by dark mode and popup. Not sure if they are really related to this PR or only just appearing now.
  • scroll-gutter
  • The creating new lists and namespaces popups should use the current page as backdrop as well (new list can be accessed through the menu from anywhere throughout the app)
  • Validate the namespace in share settings before entering the share page instead of in the template with a v-if - #816 (comment)
Here's what I think we should do in order to get this merged (other than the review comments): * [x] Error `s.available` is not a function when opening the keyboard shortcuts modal * [ ] When opening a task in the popup from list or table view and changing task attributes (like the title) the list view still has the old details in it after closing the popup. Since this won't be an easy change, I'm fine with doing that in a follow-up PR but then we should revert to the old behaviour where it opens the task in a full page instead of a modal for the list and table views. Here's what we should do related to this PR, but things that can happen in follow-up PRs: * [ ] Some style issues, probably caused by dark mode and popup. Not sure if they are really related to this PR or only just appearing now. * [ ] scroll-gutter * [ ] The creating new lists and namespaces popups should use the current page as backdrop as well (new list can be accessed through the menu from anywhere throughout the app) * [ ] Validate the namespace in share settings before entering the share page instead of in the template with a `v-if` - https://kolaente.dev/vikunja/frontend/pulls/816#issuecomment-23535
dpschen was assigned by konrad 2022-01-08 12:58:51 +00:00
konrad added 1 commit 2022-01-18 20:47:24 +00:00
continuous-integration/drone/pr Build is failing Details
e2d9aa3d7f
Merge branch 'main' into feature/vue3-modals-with-router-4
# Conflicts:
#	src/router/index.ts
#	src/views/tasks/TaskDetailView.vue
konrad added 1 commit 2022-01-18 20:51:27 +00:00
Owner

Error s.available is not a function when opening the keyboard shortcuts modal

Fixed.

> Error s.available is not a function when opening the keyboard shortcuts modal Fixed.
konrad added 1 commit 2022-01-18 21:27:57 +00:00
Owner

When opening a task in the popup from list or table view and changing task attributes (like the title) the list view still has the old details in it after closing the popup. Since this won't be an easy change, I'm fine with doing that in a follow-up PR but then we should revert to the old behaviour where it opens the task in a full page instead of a modal for the list and table views.

What should we do about this?

> When opening a task in the popup from list or table view and changing task attributes (like the title) the list view still has the old details in it after closing the popup. Since this won't be an easy change, I'm fine with doing that in a follow-up PR but then we should revert to the old behaviour where it opens the task in a full page instead of a modal for the list and table views. What should we do about this?
Author
Member

Error s.available is not a function when opening the keyboard shortcuts modal

Fixed.

Nice! You forgot the console.log

> > Error s.available is not a function when opening the keyboard shortcuts modal > > Fixed. Nice! You forgot the `console.log`
Owner

Whoops 😀

Whoops 😀
dpschen added 1 commit 2022-01-19 22:07:19 +00:00
continuous-integration/drone/pr Build is failing Details
c896ad5883
fix: subscription prop validation linting
dpschen added 1 commit 2022-01-19 22:16:55 +00:00
continuous-integration/drone/pr Build is failing Details
959b53b3a6
chore: remove console.log
dpschen added 1 commit 2022-01-19 22:26:43 +00:00
continuous-integration/drone/pr Build is failing Details
5867f79735
fix: use AsyncEditor again in comments and description
Author
Member

I can't find out why the test:

"List History should show a list history on the home page"

is failing. Any idea?

I can't find out why the test: "List History should show a list history on the home page" is failing. Any idea?
Author
Member

we should revert to the old behaviour where it opens the task in a full page instead of a modal for the list and table views.

I'll try to change that. This should be quite straight forward by removing the use of the backdropView in the routes history state (aka some param for the router when creating the detail links).

> we should revert to the old behaviour where it opens the task in a full page instead of a modal for the list and table views. I'll try to change that. This should be quite straight forward by removing the use of the `backdropView` in the routes history state (aka some param for the router when creating the detail links).
Owner

I can't find out why the test:

"List History should show a list history on the home page"

is failing. Any idea?

Is it reproducable locally?

> I can't find out why the test: > > "List History should show a list history on the home page" > > is failing. Any idea? Is it reproducable locally?
konrad added 1 commit 2022-01-30 11:40:46 +00:00
Owner

The failing test passes locally.

Here's the screenshot from CI:

image

I'm not sure why that h3 is not present either. There aren't any lists visible which may indicate a problem with the api communication.

The failing test passes locally. Here's the screenshot from CI: ![image](/attachments/0c225db8-5b7a-4211-8f35-1e818a703aee) I'm not sure why that `h3` is not present either. There aren't any lists visible which may indicate a problem with the api communication.
167 KiB
konrad added 1 commit 2022-01-30 12:07:12 +00:00
Owner

e6eb48b5af might fix the test.

Because the namespaces in the list are run through the namespaces loaded in state (because we only save their ids in history) and the home page uses this "hydrated" list of namespaces to show the history, there won't be any history shown at all if there are none in state yet. I think this was some wired race condition / flakyness. Let's hope my thesis is correct and it works now.

https://kolaente.dev/vikunja/frontend/commit/e6eb48b5afe1e4a663f1c78a113014549732d42d might fix the test. Because the namespaces in the list are run through the namespaces loaded in state (because we only save their ids in history) and the home page uses this "hydrated" list of namespaces to show the history, there won't be any history shown at all if there are none in state yet. I think this was some wired race condition / flakyness. Let's hope my thesis is correct and it works now.
konrad added 1 commit 2022-01-30 13:00:21 +00:00
continuous-integration/drone/pr Build was killed Details
3212bc8e86
fix(tests): add more waits for namespaces loaded
Owner

Didn't fix it but you can see in the screenshot from CI:

image

there's nothing loaded and it did not wait for the namespaces to load. That looks like the very first assertion for the h3 failed, not one after that.

Also it's kind of wired it fails at all because the assertion is precisely about the h3 not being present.

Didn't fix it but you can see in the screenshot from CI: ![image](/attachments/640f2580-24da-466f-9109-75ca2219575a) there's nothing loaded and it did not wait for the namespaces to load. That looks like the very first assertion for the `h3` failed, not one after that. Also it's kind of wired it fails at all because the assertion is precisely about the `h3` _not_ being present.
158 KiB
konrad added 1 commit 2022-01-30 13:03:55 +00:00
konrad added 1 commit 2022-01-30 13:10:50 +00:00
continuous-integration/drone/pr Build is failing Details
6ab7aac5ce
fix(tests): wait until lists are loaded
Owner

okay at this point I don't know what's happening

okay at this point I don't know what's happening
konrad added 1 commit 2022-01-30 13:37:44 +00:00
continuous-integration/drone/pr Build is failing Details
05350affad
fix(tests): don't assert for h3 anymore
konrad added 1 commit 2022-01-30 14:25:37 +00:00
dpschen added 8 commits 2022-01-30 15:54:03 +00:00
dpschen added 2 commits 2022-01-30 16:00:20 +00:00
dpschen added 1 commit 2022-01-30 16:24:48 +00:00
Owner

It looks like the now newly failing tests (or rather, broken functionality) comes from the new model properties. I don't know what might cause them but will revert these commits now - moved the efforts so far to https://kolaente.dev/vikunja/frontend/src/branch/feature/model-properties to let use reuse the work already done in the future.


I think having proper model properties and typing is a good idea, but I'd like to get this PR merged as soon as possible. While a nice addition, model properties don't help us in that regard.

It looks like the now newly failing tests (or rather, broken functionality) comes from the new model properties. I don't know what might cause them but will revert these commits now - moved the efforts so far to https://kolaente.dev/vikunja/frontend/src/branch/feature/model-properties to let use reuse the work already done in the future. --- I think having proper model properties and typing is a good idea, but I'd like to get this PR merged as soon as possible. While a nice addition, model properties don't help us in that regard.
konrad added 3 commits 2022-01-30 19:18:39 +00:00
konrad added 2 commits 2022-01-30 20:53:32 +00:00
konrad added 1 commit 2022-01-30 20:54:05 +00:00
continuous-integration/drone/pr Build is failing Details
56fca21320
trigger ci
Owner

I've just merged #1337 to see if my theory is correct and that was indeed the cause of the history test failing in CI.

I've just merged https://kolaente.dev/vikunja/frontend/pulls/1337 to see if my theory is correct and that was indeed the cause of the history test failing in CI.
Owner

okay well that didn't work.

I'm starting to suspect there is an actual issue in the code, not in the test. But since it only fails in CI, that makes it very hard to reproduce and fix.

okay well that didn't work. I'm starting to suspect there is an actual issue in the code, not in the test. But since it only fails in CI, that makes it very hard to reproduce and fix.
konrad added 1 commit 2022-01-30 21:11:31 +00:00
continuous-integration/drone/pr Build is failing Details
9995abf64c
fix: mark broken test as skipped
konrad added 1 commit 2022-01-30 21:33:49 +00:00
konrad added 1 commit 2022-01-30 21:48:09 +00:00
konrad added 1 commit 2022-01-30 21:51:32 +00:00
continuous-integration/drone/pr Build is passing Details
a5b0a834bc
fix(tests): make sure the namespace exists before trying to run the history tests
If there's no namespace, there is no lists in state to show in the view. The CI runs all tests from a blank state which isn't the case when running the tests locally. Therefore, if the test doesn't create a new namespace, there won't be any to test for.
konrad added 3 commits 2022-01-30 22:20:27 +00:00
Owner

I think this is the only thing that's left now to get this merged:

When opening a task in the popup from list or table view and changing task attributes (like the title) the list view still has the old details in it after closing the popup. Since this won't be an easy change, I'm fine with doing that in a follow-up PR but then we should revert to the old behaviour where it opens the task in a full page instead of a modal for the list and table views.

We essentially have two options:

  1. Put all tasks from the list in vuex so that updating one task in the popup will also update it in the list behind it
  2. Revert the task detail view to the old behaviour where it opens the task in a full page view instead of a popup when it is opened from the list view (but only then!). Given the tasks are loaded from within the taskList composable, this might be more complicated than the other one. But maybe still worth it. Maybe this is just a matter of making the returned tasks property from the composable a computed, add some state mutation and ensuring everything is committed in the list view instead of directly doing this.tasks = ...?

I don't know about the complexity of either one, I'd say we should do whatever option is faster of the two.

I think this is the only thing that's left now to get this merged: > When opening a task in the popup from list or table view and changing task attributes (like the title) the list view still has the old details in it after closing the popup. Since this won't be an easy change, I'm fine with doing that in a follow-up PR but then we should revert to the old behaviour where it opens the task in a full page instead of a modal for the list and table views. We essentially have two options: 1. Put all tasks from the list in vuex so that updating one task in the popup will also update it in the list behind it 2. Revert the task detail view to the old behaviour where it opens the task in a full page view instead of a popup when it is opened from the list view (but only then!). Given the tasks are loaded from within the taskList composable, this might be more complicated than the other one. But maybe still worth it. Maybe this is just a matter of making the returned `tasks` property from the composable a computed, add some state mutation and ensuring everything is committed in the list view instead of directly doing `this.tasks = ...`? I don't know about the complexity of either one, I'd say we should do whatever option is faster of the two.
konrad added 1 commit 2022-01-30 22:36:46 +00:00
continuous-integration/drone/pr Build is passing Details
3d420c3770
fix: make isButton prop optional
dpschen added 1 commit 2022-01-31 00:32:43 +00:00
konrad reviewed 2022-01-31 06:56:01 +00:00
@ -29,2 +36,4 @@
</router-view>
<transition name="modal">
<TaskDetailViewModal v-if="currentModal" >

I suppose this is used because we don't have a modal wrapper? Using "TaskDetailView" in the general content with component feels a little out of place IMHO 😅

I suppose this is used because we don't have a modal wrapper? Using "TaskDetailView" in the general content with component feels a little out of place IMHO 😅
Author
Member

See it like the router-view conponent of vue router: it's also not a view itself. regardless I just wanted to make the most simple change. I guess it should be renamed to something like modal-view and maybe even merged with modal 🤔 - but that can also happen later.

See it like the router-view conponent of vue router: it's also not a view itself. regardless I just wanted to make the most simple change. I guess it should be renamed to something like modal-view and maybe even merged with modal 🤔 - but that can also happen later.
Author
Member

I removed the component task-detail-view-modal in 6827390b77 and was able to merge it with the modal itself.

As a result this is obsolete now =)

I removed the component `task-detail-view-modal` in https://kolaente.dev/vikunja/frontend/commit/6827390b77ae6e186e7b0163651c19ca9a247d2f and was able to merge it with the modal itself. As a result this is obsolete now =)
dpschen marked this conversation as resolved
dpschen added 1 commit 2022-02-05 16:33:48 +00:00
continuous-integration/drone/pr Build is passing Details
6827390b77
feat: merge TaskDetailViewModal with modal
dpschen added 3 commits 2022-02-05 16:38:51 +00:00
continuous-integration/drone/pr Build is passing Details
dfa30258aa
chore: rename function
continuous-integration/drone/pr Build is passing Details
24a154422d
chore: remove vikunjaReady from store
konrad approved these changes 2022-02-05 16:40:20 +00:00
konrad merged commit a57676bf54 into main 2022-02-05 16:49:04 +00:00
konrad deleted branch feature/vue3-modals-with-router-4 2022-02-05 16:49:05 +00:00
This repo is archived. You cannot comment on pull requests.
No description provided.