feature/projects-all-the-way-down #3323
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
No Assignees
3 Participants
Due Date
No due date set.
Blocks
#2034 Remove generalisation from userTeam.vue
vikunja/vikunja
Reference: vikunja/frontend#3323
Loading…
Reference in New Issue
No description provided.
Delete Branch "feature/projects-all-the-way-down"
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?
Frontend PR for vikunja/api#1362
Test with this api url:
see: vikunja/api#1362 (comment)
Tests will fail until the api changes are merged and released as unstable. They work locally though.
@ -1,145 +0,0 @@
import {createFakeUserAndLogin} from '../../support/authenticateUser'
Many of the tests in this file still seem to make sense. They should probably be moved to projects now.
Good catch! I've added two tests from namespaces which we don't already test with projects.
@ -18,3 +14,3 @@
cy.get('.project-header [data-cy=new-project]')
.click()
cy.url()
.should('contain', '/projects/new/1')
Was that a mistake originally?
You mean the
/1
? That was the namespace where the project should be created.I think it should now be the the optional parent project.
TBH I wanted to not add these options when creating or updating a project for now. It is already possible to move a project "under" another, thus making it a child of the other. Like this:
vikunja-child-2023-03-28_16.37.45.webm
The video is 'not found'
Interesting, it works for me...
Added another comment at the bottom
Seems like I also can't paste images to upload anymore
Status Code of the video is
404
@ -0,0 +1,194 @@
<template>
<draggable
For a new component: can we use the sortable from vueuse?
Didn't know about that one, will check it out.
Would the goal here be to eventually use it everywhere instead of vuedraggable?
It looks like this is only available from vueuse 10 (we're on 9) which is not yet released as stable. I think we should wait until that's released and then move everything over.
@ -0,0 +65,4 @@
</ProjectSettingsDropdown>
<span class="list-setting-spacer" v-else></span>
</section>
<ProjectsNavigation
Nice!
@ -0,0 +94,4 @@
const props = defineProps<{
modelValue: IProject[],
allowDrag: boolean,
Should have a default value.
When is
allowDrag === true
? In case this is related to edit rights we should align the variable names.Just saw when it's true. Can we rename to the concrete action? Because the order might also be changed by something else than dragging in the future. How about
canEditOrder
?I think that's a good idea. Renamed it.
@ -207,4 +77,1 @@
}
const projectsVisible = ref<{ [id: INamespace['id']]: boolean }>({})
// FIXME: async action will be unfinished when component mounts
Can we put this component inside a
<Suspense>
? Then we can useawait
methods directly and withoutonBeforeMount
hook.I've now moved the project navigation into a separate wrapper component so that we can show a loading spinner while projects are loading and still show the other navigation links (overview, labels, etc).
Ok. Will check
@ -269,3 +82,1 @@
projectUpdating.value[project.id] = false
}
}
const projects = computed(() => Object.values(projectStore.projects)
This seems like something that the store should export instead as a computed.
That makes sense.
Changed it.
@ -272,0 +82,4 @@
const projects = computed(() => Object.values(projectStore.projects)
.filter(p => p.parentProjectId === 0 && !p.isArchived)
.sort((a, b) => a.position < b.position ? -1 : 1))
const favoriteProjects = computed(() => Object.values(projectStore.projects)
This computed as well.
@ -271,1 +82,3 @@
}
const projects = computed(() => Object.values(projectStore.projects)
.filter(p => p.parentProjectId === 0 && !p.isArchived)
.sort((a, b) => a.position < b.position ? -1 : 1))
Simplify to
Done.
@ -272,0 +88,4 @@
...p,
childProjects: [],
}))
.sort((a, b) => a.position < b.position ? -1 : 1))
Simplify to
Done.
@ -47,2 +47,2 @@
title: 'keyboardShortcuts.navigation.namespaces',
keys: ['g', 'n'],
title: 'keyboardShortcuts.navigation.projects',
keys: ['g', 'p'],
Should we keep the old shortcut and deprecate it as long as it's not used by something else?
I think it's fine to "free" it up until its used for something else without explicit deprecation.
@ -47,3 +47,3 @@
<CustomTransition name="fade">
<Message variant="warning" v-if="currentProject.isArchived" class="mb-4">
{{ $t('project.archived') }}
{{ $t('project.archivedText') }}
How about
archivedMessage
orarchivedWarning
I like
archivedMessage
, renamed it.@ -165,3 +153,1 @@
}
return !ncache[l.namespaceId].isArchived
})
return allProjects.filter((l) => typeof l !== 'undefined' && l !== null)
If projects are archived they won't show up here automatically?
yes.
We didn't include archived namespaces if I understand the code correctly.
That's correct, so now we don't show archived projects.
Okay. I still don't understand where the archived filterting happens now (didn't check in detail) but if you are aware all good :)
@ -170,13 +162,15 @@ import Nothing from '@/components/misc/nothing.vue'
import {success} from '@/message'
import {useAuthStore} from '@/stores/auth'
// FIXME: I think this whole thing can now only manage user/team sharing for projects? Maybe remove a little generalization?
👍
Not sure if we should do that here in this PR - it's narrow enough to do it later.
I created a new issue #3326
@ -71,4 +67,2 @@
emit('update:modelValue', project)
}
function namespace(namespaceId: INamespace['id']) {
This seems like a relevant info. I think we shouldn't remove this feature.
What do you mean?
We should show the parent project here now instead.
Now doing that.
I didn't have a chance yet to check the actual UI, but I realised that the parent might be 'wrong' sometimes. Or at least in the future if there are third level nested projects it might not always be the same as what one would have expected. Because historically the namespace would be shown. The latter is now the 'root ancestor'.
The way I implemented it, it will climb to the top of the tree and show each project. So if the hierarchy is something like first > second > third > task (task is a task in "third"), it will literally show "first > second > third" on the task detail view, while allowing to click on each project.
That makes sense. Hopefully there is enough space though.
@ -3,3 +3,2 @@
export function getProjectTitle(l: IProject) {
if (l.id === -1) {
export function getProjectTitle(p: IProject) {
Use long var names.
Done.
@ -25,2 +23,4 @@
position = 0
backgroundBlurHash = ''
childProjects = []
parentProjectId = 0
Coming from the recent discussion: wouldn't it be better if this was
undefined
ornull
?You mean to make it clear if this was not set, since it will be overridden with 0 by the api anyway?
Okay, wasn't aware of this. So the
0
here is coming from the golang format. I think we should use the equivalent in js for the frontend, similar to how we use camelCase for variable names.EDIT:
Keeping this
unresolved
because I still want to check something.@ -4,4 +0,0 @@
import type {IUserNamespace} from '@/modelTypes/IUserNamespace'
// This class extends the user share model with a 'rights' parameter which is used in sharing
export default class UserNamespaceModel extends UserShareBaseModel implements IUserNamespace {
UserShareBaseModel
is now only used foruserProjects
. Unify?Yeah I think we should, but let's combine that with #3326
@ -41,6 +40,7 @@ const ProjectKanban = () => import('@/views/project/ProjectKanban.vue')
const ProjectInfo = () => import('@/views/project/ProjectInfo.vue')
// Project Settings
const ListProjects = () => import('@/views/project/ListProjects.vue')
What is a general project listing for? Aren't all projects listed in the sidebar? Why do we then still need it => added functionality?
Archived projects are not listed in the sidebar. And we need to put the button to create a new project somewhere as well.
This replaces the namespace overview but instead just shows all projects.
Regarding new projects:
Shouldn't there be a button at the top or bottom of the list in the sidebar?
Archived projects => makes somehow sense. I don't like that we have a bit of a duplication with the sidebar this way, but I don't have a better either.
I don't really have a good idea on how to add one without making it look too clumsy. I think for now it's fine to have it only on the projects overview page.
A) I had an idea for the future how this could be solved:
If we let the user 'open the sidebar' meaning the sidebar 'expands to the full screen width' (doesn't mean it has to be animated; I mean this more figuratively), then the ListProjects view could be exactly that view.
So what we have is already close to this. What I'm missing are indicators in the UI, that this is what happens. Maybe there is a simpler solution to an animation. One example that I can think of is to expand the background color of the sidebar for this specific view to the full screen width.
B) The
ListProjectsProjectsNavigation
component, or at least parts of it could be used to show subprojects inside of projects.EDIT: Issue: #3338
C)
ListProjects
should share a most of its logic withProjectsNavigation
, because why shouldn't a user be able to sort the order there as well?Which one? The sidebar?
Right now it will show all projects, including child projects. But it won't show the hierarchy. Because of that, I'm not sure if we should allow reordering in its current state, simply because the hierarchy is not clearly visible.
The view that 'expands to the full screen width'.
Okay. I'll create a new issue, because I think this is something where the ux could profit when it's solved. We don't have to solve it immediately though.
EDIT: Issue: #3337
@ -203,54 +194,6 @@ const router = createRouter({
name: 'link-share.auth',
component: LinkShareAuthComponent,
},
{
We should create redirects for the old routes, so that links stay intact.
@ -110,13 +108,11 @@ export function useSavedFilter(projectId?: MaybeRef<IProject['id']>) {
async function createFilter() {
filter.value = await filterService.create(filter.value)
await namespaceStore.loadNamespaces()
Shouldn't we load the projects here now instead?
Good point, added.
@ -116,4 +113,3 @@
async function saveFilter() {
const response = await filterService.update(filter.value)
await namespaceStore.loadNamespaces()
Same here
Added.
@ -129,9 +125,8 @@ export function useSavedFilter(projectId?: MaybeRef<IProject['id']>) {
async function deleteFilter() {
await filterService.delete(filter.value)
await namespaceStore.loadNamespaces()
And here
Added
@ -20,6 +18,8 @@ export interface IProject extends IAbstract {
subscription: ISubscription
position: number
backgroundBlurHash: string
childProjects: IProject[] | null
This is really error prone. In the moment the project including child projects is coming from the backend we should replace this with an array of
childProjects
instead. All projects should be included in a flat id-Map
inside the store.Having hierarchies just adds complexity everywhere. If a component wants to use a list it can get it directly from the store.
What's that? The projects in
childProjects
are just regular projects.Coming from the api – yes I know. But using them like this in the frontend – not a good pattern. I know we did this similar in other places already. But here we this is the first time for projects. Because of that I would like to prevent the introduction of this pattern here. So when we get the regular projects child array from the api we replace them with their ids instead and add the child projects to the general projects list in the store. That general list is a list of all projects – including all child projects. If we just want the root projects we can create a filter for projects that don't have a parent.
Adding to this – of course we have to reverse this when we send stuff back to the api. By the way for all of this zod is really helpfull…
We already save all projects in the store, regardless of whether they are a child or not. The navigation then starts with a filtered list of that.
Maybe we could just ignore
childProjects
completely and only useparentProjectId
? And then build the list of child projects dynamically when needed? Not sure about the performance implications here.This is basically the idea I have only in the reverse direction. It would be better to have the id of the child though because than we don't have to iterate through all projects everytime we want to find all childs. Instead we can get via the id which should be much faster.
I wouldn't ignore the childProjects because the data inside would need to be manually synced. Instead that property should not exist in the frontend data model.
See normalizr. The utility is unmaintained but the examples are still valid. Since our store is flux inspired this applies to use as well.
The api only uses the
childProjects
property when returning a response with all projects. It won't use it to update the parent <-> child relation.Makes sense, I wonder how good that would work with the dragging and dropping of projects though.
Okay that means that we could simply return ids of the childProjects via a new property (e.g.)
childProjectIds
? That would make that step from the API obsolete.Why would that be a problem?
Note that the store still could provide a
getChildProjects
computed that would return a function where you can pass in the id of a project and would get a reactive list of child projects.I think we should be able to, yes.
Would you add that as a new property to the Project Model and then map it out in the constructor?
Not really, since we need to fetch all children anyway.
Should that include the children of children (of children... and so on) as well?
I started moving this from the current implementation to one where the store only has a flat map and does not store the children directly. It works for the basics, but I could not get any version of drag n' drop to work with that. Not sure what I did wrong.
One problem is the api returns the projects already in the child projects structure. This makes it easy to handle it as such when parsing the data from the api.
Another issue I have with that approach is how it requires a new ref in the projects navigation component, which holds all children for each project of the current tree. That's the same as a property of the
Projects
model, but feels a lot hackier.Here's what I did:
I got something working! See
2557b182dd
There are a few cases where the performance is really bad but I didn't manage to reproduce that reliably (let alone profile it).
@ -71,0 +70,4 @@
}
// if the project is a child project, we need to also set it in the parent
if (project.parentProjectId) {
See comment about having the data as sub projects from earlier.
@ -121,3 +134,3 @@
// the returned project from projectService.update is the same!
// in order to not create a manipulation in pinia store we have to create a new copy
const newProject = {
return {
Unsure: shouldn't we use klona here instead?
Not sure either, I just refactored this.
Let's use klona for this kind of stuff to be consistent.
Done.
@ -33,3 +33,7 @@ $switch-view-height: 2.69rem;
$navbar-height: 4rem;
$navbar-width: 300px;
$navbar-padding: 2rem;
These were meant as component local variables.
If we define them now as global we probably should replace them with css variables instead.
I think it's fine to use it as a scss variable since it will never change at runtime.
@ -0,0 +1,194 @@
<template>
This component is basically a redo of a huge part of what I did with https://kolaente.dev/vikunja/frontend/pulls/2108/files
Looks like it. Do you want to continue that PR? Given how it is already old and now even more outdated.
I worked many hours to untangle the CSS there. I hope that we can save that effort somehow.
From a quick glance over it, it seems like a big part of that was the namespace title styles - which are now gone (since namespaces are gone).
Probably the naming was bad. If I remember correctly the NavigationNamespace component was compatible with projects and namespaces. Not recursive though. Will check at some point how to recover good stuff.
@ -95,3 +82,2 @@
const hasTasks = computed(() => baseStore.hasTasks)
const defaultNamespaceId = computed(() => namespaceStore.namespaces?.[0]?.id || 0)
const hasProjects = computed(() => namespaceStore.namespaces?.[0]?.projects.length > 0)
const hasProjects = computed(() => projectStore.projects ? true : false)
This should be a computed of the store, like
hasTasks
.Shouldn't we check the length here (
projectStore.projects?.length
)?projects
is an object, so checking the length of it won't work.Moved it to the store.
Then
Object.keys?.length
Wouldn't that cause a call to
Object.keys
? I think my solution is faster.Checked it: https://jsbench.me/hslfseq93y/1
Using
Object.keys
really is slower, and by a lot.True. Using a Map with size would probably be the perfect solution here. But that is something for another time.
@ -63,7 +63,6 @@ async function createNewProject() {
}
showError.value = false
project.namespaceId = Number(route.params.namespaceId as string)
If we create a project inside another one doesn't it still make sense to pass the parents id down? That was the idea of that parameter.
See #3323 (comment)
It's now possible to set a parent project when creating or editing a project as well.
@ -8,4 +8,2 @@
>
<p>{{ $t('project.duplicate.text') }}</p>
<Multiselect
This should be replaced by a project selector in order to select the new parent project.
See #3323 (comment)
I don't think that in the case of duplication dragging inside the sidebar does replace that. Dragging on mobile also isn't working very well currently and has generally accessability issues.
I've now added a project select when duplicating
@ -14,4 +14,2 @@
ref="heading"
/>
<h6 class="subtitle" v-if="parent && parent.namespace && parent.project">
{{ getNamespaceTitle(parent.namespace) }} ›
Shouldn't we still show the parent here?
I've changed it now so that is shows this:
(each project is clickable individually)
For a hierarchy like this:
Can't see these images either
Again 404?
This starts to feel like a gitea bug...
@ -772,7 +756,6 @@ async function toggleFavorite() {
task.isFavorite = !task.isFavorite
const newTask = await taskService.update(task)
Object.assign(task, newTask)
await namespaceStore.loadNamespacesIfFavoritesDontExist()
It seems like this should be replaced with what you wrote above:
Maybe it would be better to create a
toggleFavorite
method of the task store instead, that includes that call so we don't forget it in other occasions when we change the favorite of a task.I thought of that as well, but completely forgot to check the task detail view. I've now created that method and replaced its usages.
You forgot to await the new store method
whoops, fixed now.
I ran through this now once. Considering the extend of this pr it would be good to make multiple rounds here. I won't have time for another round today though.
Yeah that sounds like a good idea. It's probably enough to review the new commits since you already did a full review though
Here's a video demonstration of making a project the child of another:
vikunja-child-2023-03-28_16.37.45.webm
@ -549,2 +546,2 @@
return namespaceStore.getProjectAndNamespaceById(task.projectId)
const project = projectStore.getProjectById(task.projectId)
baseStore.handleSetCurrentProject({project})
Very bad pattern.
A side effect of a computed can lead to various weird issues. Instead there should be a watcher! Why not use
getProjectById
directly?I don't even know why I moved that into the computed instead of leaving the watcher as it was before. Changed it back now.
@ -1,19 +0,0 @@
import {ref, computed} from 'vue'
This whole functionality is really nice for projects as well. Why not use it there =>
useProjectSearch
The project store has a
projectSearch
method which does something similar. Or do you have something specific in mind?@ -9,0 +14,4 @@
return project.title
}
export function getParentProjects(project: IProject, projectStore): IProject[] {
This should either be in a new helper (see the name of this file) or both should be part of the project store.
Refactored it and moved to the projects store.
@ -9,0 +16,4 @@
export function getParentProjects(project: IProject, projectStore): IProject[] {
let parents = []
if (project.parentProjectId) {
Some more stuff that I found. Will need more rounds.
@ -19,4 +19,3 @@
.should('not.contain', 'Last viewed')
cy.visit(`/projects/${projects[0].id}`)
cy.wait('@loadNamespaces')
Loading all projects in between is not necessary anymore?
No, the check which project exists will happen when displaying the history. Saving happens in a watcher on the project id in the ProjectWrapper component and thus wont need all projects to be loaded yet.
@ -0,0 +17,4 @@
await projectStore.loadProjects()
const projects = computed({
Non archived root projects seems like something that the store should provide as computed.
Done
@ -0,0 +23,4 @@
.filter(p => p.parentProjectId === 0 && !p.isArchived)
.sort((a, b) => a.position - b.position)
},
set() { }, // Vue will complain about the component not being writable - but we never need to write here. The setter is only here to silence the warning.
Probably because you use
v-model
instead ofmodelValue
for ProjectsNavigation. I guess that when the template gets rendered it wants to bind to something like@update:modelValue="(newVal) => projects.value = newVal"
I've now changed it to
:modelValue
and that seems to work.@ -0,0 +25,4 @@
},
set() { }, // Vue will complain about the component not being writable - but we never need to write here. The setter is only here to silence the warning.
})
const favoriteProjects = computed(() => projectStore.projectsArray
favoriteProjects
also seems like something a store should export.Done
@ -0,0 +27,4 @@
})
const favoriteProjects = computed(() => projectStore.projectsArray
.filter(p => !p.isArchived && p.isFavorite)
.map(p => ({
Why the mapping / returning of a copy?
That was a relic, I've removed it now.
@ -469,3 +136,1 @@
overflow: hidden;
margin-bottom: 0;
flex: 1 1 auto;
.loader-container {
This class doesn't isn't used in the template
I've renamed it - it is coming from the
Loader
component.2cf1e91b63
to733f0c1e19
@ -0,0 +56,4 @@
>
<icon :icon="p.isFavorite ? 'star' : ['far', 'star']"/>
</BaseButton>
<ProjectSettingsDropdown class="menu-list-dropdown" :project="p" v-if="p.id > 0">
Didn't we just remove this condition, so that also one can also adjust settings of favorited lists?
The condition is already in main: https://kolaente.dev/vikunja/frontend/src/branch/main/src/components/home/navigation.vue#L132
But editing favorites works just fine. This is about every project which actually exists, so no shared filters for example.
My bad I confused favorite projects with the 'Favorite' project.
@ -0,0 +66,4 @@
<span class="list-setting-spacer" v-else></span>
</section>
<ProjectsNavigation
v-if="!collapsedProjects[p.id]"
Also don't render if there are no child projects.
But then it won't be possible to drag a project "under" a parent to make it a child of that parent.
@ -0,0 +1,30 @@
<template>
<nav class="menu" v-if="favoriteProjects">
Didn't check / thought: I think to remember that
menu
is a class also coming from bulma.If that's the case: does that mean that we pass these styles down here? Could we instead add the menu class inside ProjectsNavigation or wouldn't that work because of the recurrsion?
Yes,
.menu
is a class from bulma.You mean adding the class into the styles definition of the
ProjectsNavigation
component?@ -20,0 +19,4 @@
<Suspense>
<navigation class="d-print-none"/>
<template #fallback>
Loading...
Use
components/misc/loading.vue
That was an old test I did, I've now removed it.
@ -5,3 +5,3 @@
<Logo width="164" height="48"/>
</router-link>
<ul class="menu-list">
<ul class="menu-list other-menu-items">
Something that I don't know that long myself: adding a
<ul>
inside a<nav>
doesn't give added benefit if we hide thelist-item
, because there are no added semantics. So listing the links is ok.So we could get rid of the
ul
entierly and only add the links?That was my idea. But wait with doing this. Because when I wrote this I forgot that menu-list adds style the is structure dependant (might even be that the original class is coming from bulma).
If Bulma doesn't require you to use
<ul>
use<menu>
instead.menu
still missing here. (you changed it in ProjectsNavigation).Done
@ -432,4 +115,2 @@
}
.top-menu {
margin-top: math.div($navbar-padding, 2);
Remove outer margin set from inside the component and add it from outside instead.
@ -0,0 +1,134 @@
// these are general menu styles
// should be in own components
.menu {
.menu-label,
menu-label
isn't used anymore.Removed.
@ -9,3 +9,3 @@
it('should show a project history on the home page', () => {
cy.intercept(Cypress.env('API_URL') + '/namespaces*').as('loadNamespaces')
cy.intercept(Cypress.env('API_URL') + '/projects*').as('loadProjects')
Rename to
loadProjectArray
to make the vars better distinguishable.@ -15,3 +14,1 @@
.click()
cy.get('.namespace-title .dropdown .dropdown-item')
.contains('New project')
cy.visit('/projects')
Should we add a test for projects created from the sidebar?
But you can't create a project from the sidebar?
it was possible in the contextmenu of namespaces
Yes, but well namespaces are gone now. We could add an option to the project contextmenu? That won't allow to create new root level projects though.
@ -0,0 +6,4 @@
@start="() => drag = true"
@end="saveProjectPosition"
handle=".handle"
tag="ul"
Use
<menu>
(now I remembered the correct element).Done
@ -0,0 +18,4 @@
]
}"
>
<template #item="{element: p}">
Use long var name. I thought for a bit that for
<draggable>
you can define the type of dom node of the child item viaelement
.Done.
@ -0,0 +24,4 @@
:class="{'is-loading': projectUpdating[p.id]}"
:data-project-id="p.id"
>
<section>
Since this block doesn't have a headline it shouldn't be a
<section>
. Maybe use<nav>
instead (nesting is allowed!)Move this whole block in a new
ProjectNavigationItem.vue
component. Reduces also the whole complexity withchildProjects[p.id]
because we can pass only the project.Done
Shouldn't a
nav
hold multiple navigation items?Yes! Sry I misread the position, where the
<section>
is.Okay you moved now only the item without the list below inside.
What i meant was:
<li>
insideProjectsNavigationItem.vue
.ProjectsNavigation.vue
is then used inside ProjectsNavigationItemThis whole block can then be simplified:
Because we can save the collapsed state inside each item we don't need to manage a list anymore.
So we don't even need the
<section>
then and can instead use the<li>
.It's totally fine to not group the buttons etc because they are already grouped by the
<li>
they are in. TheProjectsNavigation
component would be the last child insieProjectsNavigationItem
That makes sense. I've moved most of the logic over, as you suggested.
We actually need this (or another element) because the
section
is a flexbox container for the project title and related buttons. We can't use theli
as the flexbox container because the ProjectsNavigation for the child projects needs to stay below the project title etc. If it was in the same flexbox container it would get pushed to the right.@ -0,0 +110,4 @@
// Vue draggable will modify the projects list as it changes their position which will not work on a prop.
// Hence, we'll clone the prop and work on the clone.
// FIXME: cloning does not work when loading the page initially
What consequence has this?
This now works, it didn't in an earlier version. I've removed the comment.
@ -0,0 +111,4 @@
// Vue draggable will modify the projects list as it changes their position which will not work on a prop.
// Hence, we'll clone the prop and work on the clone.
// FIXME: cloning does not work when loading the page initially
// TODO: child projects
What exactly needs to be done for child projects?
That was an old comment, I've removed it
@ -0,0 +15,4 @@
const projectStore = useProjectStore()
await projectStore.loadProjects()
Call this in
App.vue
and add aisLoadingPromise
that contains the returned promise of theloadProjects
function to the projectStore which we then can await here instead.What would be the advantage of this? I feel like this would be a lot more complicated.
The loading of the projects would happen much earlier in the request chain.
Since they are an async dependency the optimal time to request the server would be the moment we know that we have are authenticated.
So I guess it Shouldn't be App but maybe contentAuth.
That's also the reason why we load all labels there.
I've now moved it to
contentAuth
but am using theisLoading
property of the project store to show the loading state. That means I can't use theSuspense
component, but the loading property already exists and is populated, might as well use it.That makes sense.
Now
ProjectsNavigationWrapper.vue
is so simple that it doesn't make sense to have it as dedicated component. The reason why it was created was for theSuspense
, right?Yes, I created it only to use it with
Suspense
. Should I include its contents back intonavigation
?I think that would be better now. Sry for the back and forth.
No worries, changed it back.
@ -0,0 +25,4 @@
<style lang="scss" scoped>
.menu {
padding-top: math.div($navbar-padding, 2);
The top menu shouldn't have a padding top that has the sole purpose of creating space to the silbling component above. Since I still only check the code and don't have this branch running locally I'm not sure if the padding extends the background or something like that instead.
If not this
.menu
should use margin-top:Done.
@ -494,3 +140,1 @@
color: var(--warning);
opacity: 1;
}
&.is-loading::after {
This changes styles inside the component. If the styles need to be adjusted the component should be changed instead.
The problem is the component is used in multiple places where this would need different sizes. The way to do this would probably be variants with a prop?
Correct! The question to ask is also: do we even need so many different sizes or shouldn't they be more unied. For now I guess it's enough to answer this quesion for this usecase here.
I've now added this as a variant to the component. In doing this I discovered there are more styles and uses of that loader which we should refactor at some point. I would consider that out of scope for this PR though.
@ -153,9 +140,6 @@ async function unsubscribe() {
let message = ''
switch (props.entity) {
Add type to props definition: Should be
ISubscription['entity']
Done.
@ -53,3 +58,1 @@
?.filter(value => value > 0)
.map(id => projects.value[id])
.filter(project => project.isArchived === includeArchived)
?.filter(value => value > 0)
Remove added indention
Done.
I've added a nesting limit.
@ -0,0 +45,4 @@
canEditOrder: boolean,
canCollapse?: boolean,
}>()
const emit = defineEmits(['update:modelValue'])
Add types for emit
Done
@ -0,0 +48,4 @@
const emit = defineEmits(['update:modelValue'])
const drag = ref(false)
const dragOptions = {
These options should either contain all dragOptions or be defined inline
Moved it all inline.
@ -0,0 +55,4 @@
const projectStore = useProjectStore()
// Vue draggable will modify the projects list as it changes their position which will not work on a prop.
Is this even necessary if we use
modelValue
instead ofv-model
for the draggable?v-model
is required, usingmodelValue
for the draggable component does not work.@ -0,0 +2,4 @@
<li
class="list-menu loader-container is-loading-small"
:class="{'is-loading': isLoading}"
:data-project-id="project.id"
Set from outside, since this id is related to the sorting.
Done
@ -0,0 +4,4 @@
:class="{'is-loading': isLoading}"
:data-project-id="project.id"
>
<section>
Replace section with
<div>
.We'll add correct semantics here later (e.g. https://www.w3.org/WAI/ARIA/apg/patterns/menubar/examples/menubar-navigation/).
<section>
is not correct though, since there is no headline.Done
@ -0,0 +18,4 @@
class="list-menu-link"
:class="{'router-link-exact-active': currentProject.id === project.id}"
>
<span class="icon menu-item-icon handle">
Fix indention
Pass 'handle class name from parent => separate concerns / source of truth
But the indention is correct?
Can you explain that a little more?
The
handle
selector is used in the child. Currently we define it in the parent. We should pass this information down to the child. Might also be via creating a slot in the child where we put in the handle.@ -0,0 +46,4 @@
<span class="list-setting-spacer" v-else></span>
</section>
<ProjectsNavigation
v-if="childProjectsOpen"
Use
Expandable
component for this.This seems to completly break the styling. I changed it to match the selectors but it still does not work. Not sure what to do about this.-
I created an example how to use this in
51e29af010
. I was unsure which parts parts should be dynamically be filled (via theopen
prop) or static (not rendering theExpandable
at all viav-if
).@ -142,3 +51,1 @@
</draggable>
</template>
</nav>
<Loading variant="small" v-if="projectsLoading"/>
Move
v-if
to frontDone
@ -202,2 +78,3 @@
const projectStore = useProjectStore()
const menuActive = computed(() => baseStore.menuActive)
const projectsLoading = computed(() => projectStore.isLoading)
Remove both computed above and use store + property directly instead.
Done
@ -561,3 +143,1 @@
.namespaces-list.loader-container.is-loading {
min-height: calc(100vh - #{$navbar-height + 1.5rem + 1rem + 1.5rem});
.menu + .menu{
Missing space
Done
Can't rename a project via
Contextmenu => Edit => new name => save
Inbox shouldn't be able to have children. Seems very weird that this is possible
=> For now we keep the current implementation
It shouldn't be possible to drop a project as a child of itself. This is possible if it's in the sidebar a second time, because it's marked as favorite.
=> For now we keep the current implementation
Inbox should always be on top.
=> For now we keep the current implementation
It shouldn't be possible to make
Inboxthe default project a child of another project.separating headline is missing. It was good that I could see where my favorites start. Maybe instead of readding the headline we should at least put the a separator between each groups. The first item in the favorites group might be the inbox, so that it's always a 'favorite' – even though a user shouldn't be able to change the favorite state.
color dot OR icon should be handle. Because right now it's very confusing on which level a project is
inbox on same level as "project 2":
Not enough spacing to button inside
/projects
:restoring an archived subproject shouldn't be possible, because it destroys the parent child relationshop. Possible alternative: only make parent restorable.
active project is missing blue line on side indicating that it's currently open
size of clicking area got reduced: left of handle isn't clickable anymore. Easy to see if you compare the focus state:
old
new
@ -54,6 +54,7 @@ ENV VIKUNJA_LOG_FORMAT main
ENV VIKUNJA_API_URL /api/v1
ENV VIKUNJA_SENTRY_ENABLED false
ENV VIKUNJA_SENTRY_DSN https://85694a2d757547cbbc90cd4b55c5a18d@o1047380.ingest.sentry.io/6024480
ENV VIKUNJA_INFINITE_PROJECT_NESTING_ENABLED false
Add types to
env.d.ts
Even though this env variable only works in Docker and is translated to a window. variable at runtime? We don't do this for the other variables either...
@ -0,0 +95,4 @@
if (project.parentProjectId !== parentProjectId && project.parentProjectId > 0) {
const parentProject = projectStore.getProjectById(project.parentProjectId)
const childProjectIndex = parentProject.childProjectIds.findIndex(pId => pId === project.id)
parentProject.childProjectIds.splice(childProjectIndex, 1)
Danger! This should be handled in the store!
Inline editing of parent project!
Probably it would be best to create a new store method. Something like
setOrder
orchangeOrder
.I was able to move the whole thing into the
updateProject
method of the store.@ -0,0 +105,4 @@
position,
parentProjectId,
})
emit('update:modelValue', availableProjects.value)
Really unsure here: Since we do map data directly from the store I'm unsure if we should emit and update the store. Because an emit will change data in the parent. An update from the store will probably as well.
It looks like it works fine without the emit. We're always binding with
:modelValue
instead ofv-model
anyway…@ -0,0 +67,4 @@
import ColorBubble from '@/components/misc/colorBubble.vue'
import ProjectsNavigation from '@/components/home/ProjectsNavigation.vue'
const props = defineProps<{
Define default types or handle undefined defaults. E.g.
level
might beundefined
.Could it be that ts doesn't display errors in the editor for you?
Added.
Looks like it 🤔
@ -36,2 +36,4 @@
.filter(p => !p.isArchived && p.isFavorite))
const hasProjects = computed(() => projects.value ? true : false)
const getProjectById = computed(() => {
This computed seems really unnecessary. Reason: We can achieve the same (and faster) by using:
projects.value[id]
. Sinceprojects
is exported we should replace uses of this computed. We might need to create new simple computeds where used. Depending on usecase something likeWe've actually been using computed for most uses of the store computed anyway. I've changed it to use the
projects
property of the store directly.@ -38,2 +40,4 @@
return (id: IProject['id']) => typeof projects.value[id] !== 'undefined' ? projects.value[id] : null
})
const getChildProjects = computed(() => {
return (id: IProject['id']) => projectsArray.value.filter(p => p.parentProjectId === id) || []
The fallback is unnecessary OR the type of the projects ref is wrong. Because
Object.values
on an object should always return an array.The type is correct. I've removed the fallback.
The new type on the computed is wrong. The computed returns a function that returns
IProject[]
. So the correct type is probably(id: IProject['id']) => IProject[]
.But that should be automatically inferred by the returned function. If it is not inferreed, annotate the returned function instead and remove the type annotation of the computed.
Should be fixed now.
@ -196,1 +227,4 @@
)
async function save() {
project.parentProjectId = parentProject.value.id
This changes project before calling update. was that intended? What happens if
parentProject.value.id
is undefined?It was not intended. I've changed it now so that it checks it before and provides a proper fallback.
@ -71,10 +76,7 @@ export const useProjectStore = defineStore('project', () => {
}
The three lines above should be called via a deep watcher on the current project. Not as a side effect.
You mean setting the current project in the base store?
I mean:
setProject
function:It might be even better to make the currentProject a computed based on the store instead and only setting it via id (unsure here how to handle projects that are not saved yet)
I've now changed it to use a watcher.
That sounds like a good idea, but let's push that to another PR.
I've now changed it to use a watcher.
That sounds like a good idea, but let's push that to another PR.
@ -36,0 +34,4 @@
.filter(p => p.parentProjectId === 0 && !p.isArchived))
const favoriteProjects = computed(() => projectsArray.value
.filter(p => !p.isArchived && p.isFavorite))
const hasProjects = computed(() => projects.value ? true : false)
Since
projects
is of type object (defined by its type) this shouldn't work because!!{} === true
.Even if it would be undefined or null sometimes this should use
Boolean(projects.value)
for clarity instead.Afaik there is no way around something like:
Fixed.
@ -100,8 +102,6 @@ export const useProjectStore = defineStore('project', () => {
try {
const createdProject = await projectService.create(project)
createdProject.namespaceId = project.namespaceId
If I set an id for
parentID
in the passed project: will this update the parents childProjects?It did not. While checking this, I discovered how we're not using child ids anywhere any more (including in the api) so I've removed all traces of them.
@ -116,22 +116,10 @@ export const useProjectStore = defineStore('project', () => {
try {
await projectService.update(project)
Use returned value from update function. Then klona shouldn't be necessary, right?
If I set an id for
parentID
in the passed project: will this update the parents childProjects?Done.
@ -162,0 +155,4 @@
projects.value = {}
setProjects(loadedProjects)
return loadedProjects
Unsure: we might want to return the return value of setProjects here instead. Because maybe the projects will be modfied while being set. So it will be better to return from that method OR filter and return the projects with the id from the store object.
But
setProjects
does not return anything? Neither doessetProject
.These methods don't modify anything. I think it's fine to leave it like it is.
@ -162,0 +161,4 @@
}
}
function getParentProjects(project: IProject): IProject[] {
When I read this first I thought that this implies that one project could have several parents. If I got it correct that's wrong. So how about
getAncestors
instead?I like that. Changed it.
@ -194,2 +215,4 @@
const projectStore = useProjectStore()
const parentProject = ref<IProject | null>(null)
If the parent should be editable the parent should use
useProject
itself. I our target here is to make the parentProjectId updateable: why do we need to export an extra object, since the parentProjectId is already part of the project? If not: could we use a computed getter / setter here instead?I've moved it out of the composable. It's only used in one place anyway...
@ -10,2 +10,3 @@
@import 'helpers';
@import 'helpers';
@import 'navigation';
Since these styles are only used in the navigation component: import them only there
This only works if the imported styles are not scoped to the navigation component. I could still import them in a non-scoped
style
tag in that component but I guess it's probably better to refactor this whole thing at some point so that it uses individual components instead of one global style sheet (not in this PR)@ -20,4 +20,0 @@
<template v-if="!hasTasks && !loading">
<template v-if="defaultNamespaceId > 0">
<p class="mt-4">{{ $t('home.project.newText') }}</p>
<x-button
Why did we remove this button?
Because it is related to having a namespace and no projects. With the new changes, there will always be at least one project for the user, which means this would never be shown anyway.
@ -95,3 +82,2 @@
const hasTasks = computed(() => baseStore.hasTasks)
const defaultNamespaceId = computed(() => namespaceStore.namespaces?.[0]?.id || 0)
const hasProjects = computed(() => namespaceStore.namespaces?.[0]?.projects.length > 0)
const hasProjects = computed(() => projectStore.hasProjects)
Don't wrap computed in computed. use
projectStore.hasProjects
directlyDone
@ -0,0 +20,4 @@
:show-archived="showArchived"
/>
<!-- <p v-if="projects.length === 0" class="has-text-centered has-text-grey mt-4 is-italic">-->
Why is this commented out?
Because when I started this PR I was unsure if I might want to bring back pieces of it. Then I just forgot to remove it (done now).
@ -0,0 +90,4 @@
const showArchived = useStorage('showArchived', false)
const loading = computed(() => projectStore.isLoading)
const projects = computed(() => {
Create a new exported computed from the store
archivedProjects
. We can switch between the normal projectsArray and the archived computed when returning here.But this shouldn't show only archived projects but archived and unarchived. Having a computed called
archivedProjects
would sound like it only shows archived projects which does not really make sense.Isn't that what
projectsArray
already is (list of archived and unarchived projects)?This way we don't have to filter it if we don't need to.
Good catch, I've now changed it.
@ -55,6 +63,7 @@ const showError = ref(false)
const project = reactive(new ProjectModel())
const projectService = shallowReactive(new ProjectService())
const projectStore = useProjectStore()
const parentProject = ref<IProject | null>(null)
Use a computed getter / setter for parent project via
project.id
.But this is only used to store the selection of the new parent project? That will never change from anywhere other than the project search component.
See this comment: #3323 (comment)
Main point is: source of truth should be url. And url will set parentId inside the
project
reactive. TheparentProject
should be a computed that is filled based on that id. If the id changes (everything else shouldn't change) that id will be updated inside theproject
reactive. I guess it would also be nice to replace / update the url as well but that was not my main point here. Edit: see also this #3323 (comment)@ -56,3 +64,4 @@
const projectService = shallowReactive(new ProjectService())
const projectStore = useProjectStore()
const parentProject = ref<IProject | null>(null)
Add immediate watcher from route. Watch for parentProjectId.
But the route will never have a project id? (or parent project id for that matter)
The route is always
/projects/new
and it's not possible to create a project from the context menu of an existing project.We do remove functionality here if we disallow this.
Because with namespaces it was possible to say in which namespace I wanted to create a new list via a url parameter. Since root projects are the pendant of the old namespaces we should keep the functionality to add new projects inside others directly.
I've now added this. It may not be the most optimal version but I think it's good enough.
@ -63,2 +72,4 @@
}
showError.value = false
if (parentProject.value) {
If we have the computed getter / setter this shouldn't be necessary
@ -65,4 +78,3 @@
project.namespaceId = Number(route.params.namespaceId as string)
const newProject = await projectStore.createProject(project)
await router.push({
Push to new project route in store.
Done.
@ -57,3 +36,3 @@
const namespaceStore = useNamespaceStore()
const projectDuplicateService = shallowReactive(new ProjectDuplicateService())
const parentProject = ref<IProject | null>(null)
Use
useProject
to get current project here and addduplicateProject
method.Done.
@ -97,3 +104,3 @@
const {t} = useI18n({useScope: 'global'})
const {project, save: saveProject, isLoading} = useProject(props.projectId)
const {project, parentProject, save: saveProject, isLoading} = useProject(props.projectId)
Use computed getter / setter to set parentProjectId of project instead.
I've changed this since moving the parent project logic out of the composable.
@ -19,2 +16,2 @@
{{ getProjectTitle(parent.project) }}
</router-link>
<h6 class="subtitle" v-if="project?.id">
<template v-for="p in projectStore.getParentProjects(project)">
This won't update dynamically. Should we change the
getParentProjects
to a computedparentProjects
that automatically updates instead?I tried to change it but it fails with
getAncestors is not a function
. Any idea?Actually this does update dynamically when the project changes. I've had a task open, moved the project to another parent project and it updated instantly.
@ -21,0 +18,4 @@
<router-link :to="{ name: 'project.index', params: { projectId: p.id } }">
{{ getProjectTitle(p) }}
</router-link>
<span class="has-text-grey-light" v-if="p.id !== project.id"> > </span>
Don't end with the error sign. Only use it in between two ancestors or as separator to task title if the latter is direclty next to it.
But that's what this does?
@ -548,3 +539,1 @@
}
return namespaceStore.getProjectAndNamespaceById(task.projectId)
const project = computed(() => task.projectId ? projectStore.getProjectById(task.projectId) : null)
getProjectById
returnsundefined
if there is no project with that id. So why not use that directly?Done
@ -47,2 +47,4 @@
this.subscription = new SubscriptionModel(this.subscription)
}
this.childProjectIds = this.childProjects?.map(p => p.id) || []
If we receive child projects like this from the server: Do we have that info somewhere else?
We don't actually receive them like this anymore, so this is obsolete, and I've removed it.
Each project has a parent project id. To get all child projects we need to iterate over them and return all projects with a parent project id of the project we're interested in. We don't need to that anywhere so I think it's fine to leave at that.
@ -6,3 +5,3 @@
projectId: number
namespaceId: INamespace['id']
project: IProject
parentProjectId: number
IProject['id']
Done.
@ -23,6 +23,7 @@ declare global {
API_URL: string;
SENTRY_ENABLED: boolean;
SENTRY_DSN: string;
INFINITE_PROJECT_NESTING_ENABLED: boolean;
PROJECT_INFINITE_NESTING_ENABLED
Done.
I was not able to reproduce this.
The inbox is just the first project which gets created when the user creates their account. It's not special in any way other than that. We could also change the name, but I think "inbox" is something people are familiar with. To avoid the complexity, I'd like to avoid making it special, since that would mean more complexity in the api as well…
That's the kind of additional complexity I meant…
How were you able to do that? It's not possible to drag favourites (at the top) or drag favourite projects into a child project.
Then we also should add a separator between the links on top. I think only adding a line would feel out of place because there are no lines anywhere in the menu. What about adding more space between them?
I noticed that, but couldn't figure out a solution. I like the idea, changed it. Let me know what you think.
Fixed.
You mean if the parent is archived?
I removed that on purpose and instead colour the text. I don't think it looks great because it would "float" on the left due to the nesting.
Fixed.
Yes that makes sense.
Users are also familar with the inbox being special and being always the first item. So it's a bit more complex than that. For now: how about indicating somehow that the inbox is the current default project? Afaik this is not visible anywhere other than the settings.
Might be that I had an older version
Maybe I miscommunicated here. What I meant was:
We removed the 'FAVORITES' headline that separated the favorites from the menu above. With spacing alone we can't create the same level of separation as we had before (again trying to keep 'functionality' here). Namespaces had different font settings than normal projects. My idea was that instead of changing font settings of the root project and or adding the 'FAVORITES' headline back, we could add a small light line in between those sections. By section I meant only: Overview–Teams, Favorites, Projects. If there are no favorites only line less will be needed. Maybe the lines don't look good, but we might be able to adjust them, so that they fit.
Regarding Dot as handle
Should have specified better what I meant here: if the user is dragging / while hovering the 'dot' I imagined that the handle should be displayed instead. To be able to do that without flickering the reserved area (not actual size) of the dot and handle should be identical.
Correct! I archived a root project that had one sub project. Then i restored the sub project that was marked as archived. If I remember correctly it showed up in the sidebar. I think it shouldn't be possible to unarchive a child if its current parent is archived as well.
I don't think this at all. We also didn't have this issue with the old sidebar. There we already had two levels (namespaces and lists).
Displaying more than two levels will always be tricky in the sidebar, because further indention further decreases the little space that we have to display the title. That's why file explorer usually don't allow this. Because of this further (visual) indention shouldn't happen even if we continue to allow further nesting.
If you hover subprojects there is now an area to the left that is not highlighted. This is an unnecessary change from old lists inside namespaces. The whole width of the sidebar should be highlighted regardless of the nesting level.
@ -0,0 +91,4 @@
.sort((a, b) => a.position - b.position)
})
const canNestDeeper = computed(() => {
Simplify:
But that would return
false
for the first two levels?@ -269,3 +88,1 @@
projectUpdating.value[project.id] = false
}
}
const projects = computed(() => projectStore.notArchivedRootProjects
This sorts the returned computed of the store
=> sort already in store OR create copy (worse performance)
Error in vue developer tools:
Now sorting in store, that seems to work (or at least there are no errors now)
@ -272,0 +88,4 @@
const projects = computed(() => projectStore.notArchivedRootProjects
.sort((a, b) => a.position - b.position))
const favoriteProjects = computed(() => projectStore.favoriteProjects
.sort((a, b) => a.position - b.position))
This sorts the returned computed of the store
=> sort already in store OR create copy (worse performance)
Error in vue developer tools:
Fixed as well.
@ -22,6 +22,12 @@
<p class="help is-danger" v-if="showError && project.title === ''">
{{ $t('project.create.addTitleRequired') }}
</p>
<div class="field">
Hide field if there are no other projects. Maybe show note instead
Now hiding it.
@ -561,3 +149,1 @@
.namespaces-list.loader-container.is-loading {
min-height: calc(100vh - #{$navbar-height + 1.5rem + 1rem + 1.5rem});
.menu + .menu {
Unsure if this was this line, but: The space between the logo and the main sidebar nav items got reduced. The indention of the text (including icons) as well. Text indention is fine (because the icons seem to align with logo), vertical spacing is not!
Re-added the space to the logo
Where would you indicate that? I don't think we should add yet another icon to the sidebar but maybe in the projects overview?
Done that now.
With the old sidebar we didn't have nested menu items. The problem here comes from the structure of these and the indention of sub projects.
I would like to change the active and hover state into something like this, but want to get the functional things working here first:
That's what I meant above. It sure is possible to make that work, it would just add more complexity for something that will change anyway.
I tried that, but it did not work as expected:
The UI could be better here (because the archived child project did not show up as being archived) but it seems to work as you'd expect.
Agreed, but that should be enforced by the api. I'll add a check.
I think we should do this outside of this PR, because it's already "broken" on the current main branch so it should be fixed there. I've added a task to the roadmap.
Hi konrad!
Thank you for creating a PR!
I've deployed the changes of this PR on a preview environment under this URL: https://3323-feature-projects-all-the-way-dow--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!
1731969898
to9241c7e587
9241c7e587
toed8de7e3eb
From my point of view all comments are addressed and the tests are green. Merging now!