feat: implement modals with vue router 4 #816
No reviewers
Labels
No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
No Milestone
No project
3 Participants
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/frontend#816
Loading…
Reference in New Issue
No description provided.
Delete Branch "dpschen/frontend:feature/vue3-modals-with-router-4"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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
That sounds really awesome.
@ -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?
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.
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).
Sidenote: stumbled upon this related thread...
78834283d8
to3fb1e2ea2b
3f1988d723
to8b43bfe224
8b43bfe224
to6830d8e0c1
@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
8eb2e3f281
to6f4fea6ffc
You mean #926 ?
Yes and with #899
6f4fea6ffc
to3f1cecca7b
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!
wip: feat: implement modals with vue router 4to feat: implement modals with vue router 4@konrad: can you help me fix the remaining issues here =)
Sure! Do you have a rough list of things that need fixing? Or just replacing every modal view to use the new syntax?
I thought it's mainly fixing the tests now.
Changing the other modals should be a different task.
97133107d1
to68b6fb8f38
ec74b1e9a0
toc63991b3a8
@konrad:
Tests that I fail to fix and have no clue where the issue is coming from:
sharing/linkShare.spec.jsuser/login.spec.jsuser/registration.spec.jsThe rest of the failing tests could be solved via the
getSettled
workaround.314b2f5935
to54aaa4c836
54aaa4c836
to1296f1c748
1296f1c748
to99691cc149
99691cc149
to011207dfb5
598362a859
to7e4a57b13e
@ -10,1 +10,3 @@
}
},
"testFiles": [
"list/list.spec.js",
Why is the
list.spec.js
seperated from the rest?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.
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
Sometimes it's really cool, if you know JSON #fail
(was a line end comma)
@ -265,2 +265,4 @@
overflow-y: auto
}
@include modal-transition();
Doesn't repeating this create duplicate css everywhere? (even if very little)
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 =)
@ -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?
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.
Done
@ -1,102 +0,0 @@
import TaskCollectionService from '@/services/taskCollection'
Oh you got rid of the mixin, very nice!
@ -0,0 +21,4 @@
const loading = computed(() => taskCollectionService.value.loading)
const totalPages = computed(() => taskCollectionService.value.totalPages)
const tasks = ref([])
Please indent.
Done
There's this error happening when opening a task from the list view:
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.
7f97c34787
to08aa701924
Seems to be the same as https://sentry.io/organizations/vikunja/issues/2810598130/?project=6024480&query=is%3Aunresolved&statsPeriod=90d
08aa701924
to7b4bb80ec3
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#L127Nope, still happening :/
e922f3beed
tocdffc71298
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
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.
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 =)
@dpschen and in what view? (list, Gantt, Kanban etc.) There could be cases where this is not saved in local storage just yet.
@ -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())
Maybe shallowReactive fits here better?
@ -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.
TODO: check if this works
@ -504,0 +321,4 @@
return {
name: router.hasRoute(savedListView)
? savedListView
: 'list.list',
@konrad: is defined here
Ah okay that makes sense.
@ -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
Remove this
f8d57188e7
to1a5032d8c6
1a5032d8c6
to6b59d2a9c9
6b59d2a9c9
to43c935ca41
43c935ca41
toe6e8a98514
@ -1,4 +1,5 @@
<template>
<!-- FIXME: transition should not be included in the modal -->
Where should it be included instead?
Around the modal. So where it will get mounted.
Ah okay, makes sense.
@ -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?
It does have a modal inside! See line 135.
By importing it here we remove the indirect dependency.
@ -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?
I was probably going to far here – will undo.
I guess loading the editor async might even make sense with a new smaller package.
@ -0,0 +1,185 @@
<template>
If this isn't accessible as a "view" it should not go in
src/views/
I see the views folder more defined as something like
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.
@ -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?
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).
Here's what I think we should do in order to get this merged (other than the review comments):
s.available
is not a function when opening the keyboard shortcuts modalHere's what we should do related to this PR, but things that can happen in follow-up PRs:
v-if
- #816 (comment)Fixed.
What should we do about this?
Nice! You forgot the
console.log
Whoops 😀
I can't find out why the test:
"List History should show a list history on the home page"
is failing. Any idea?
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).Is it reproducable locally?
The failing test passes locally.
Here's the screenshot from CI:
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.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.
Didn't fix it but you can see in the screenshot from CI:
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.okay at this point I don't know what's happening
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.
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.
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.
I think this is the only thing that's left now to get this merged:
We essentially have two options:
tasks
property from the composable a computed, add some state mutation and ensuring everything is committed in the list view instead of directly doingthis.tasks = ...
?I don't know about the complexity of either one, I'd say we should do whatever option is faster of the two.
@ -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 😅
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.
I removed the component
task-detail-view-modal
in6827390b77
and was able to merge it with the modal itself.As a result this is obsolete now =)