feature/projects-all-the-way-down #3323

Merged
konrad merged 123 commits from feature/projects-all-the-way-down into main 2023-05-30 10:09:40 +00:00
Owner

Frontend PR for vikunja/api#1362

Test with this api url:

https://projects.vikunja.dev/api/v1

see: vikunja/api#1362 (comment)

Frontend PR for https://kolaente.dev/vikunja/api/pulls/1362 Test with this api url: ``` https://projects.vikunja.dev/api/v1 ``` *see: https://kolaente.dev/vikunja/api/pulls/1362#issuecomment-48197*
dpschen was assigned by konrad 2023-03-28 12:53:41 +00:00
konrad added 28 commits 2023-03-28 12:53:42 +00:00
konrad requested review from dpschen 2023-03-28 12:53:47 +00:00
Author
Owner

Tests will fail until the api changes are merged and released as unstable. They work locally though.

Tests will fail until the api changes are merged and released as unstable. They work locally though.
dpschen reviewed 2023-03-28 13:04:20 +00:00
@ -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.

Many of the tests in this file still seem to make sense. They should probably be moved to projects now.
Author
Owner

Good catch! I've added two tests from namespaces which we don't already test with projects.

Good catch! I've added two tests from namespaces which we don't already test with projects.
dpschen marked this conversation as resolved
dpschen reviewed 2023-03-28 13:06:38 +00:00
@ -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?

Was that a mistake originally?
Author
Owner

You mean the /1? That was the namespace where the project should be created.

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.

I think it should now be the the optional parent project.
Author
Owner

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

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](/attachments/b05a1bfa-5471-4555-b355-eec7057053cc)

The video is 'not found'

The video is 'not found'
Author
Owner

Interesting, it works for me...

Interesting, it works for me...
Author
Owner

Added another comment at the bottom

Added another comment at the bottom

Seems like I also can't paste images to upload anymore

Seems like I also can't paste images to upload anymore

Status Code of the video is 404

Status Code of the video is `404`
konrad marked this conversation as resolved
dpschen reviewed 2023-03-28 13:08:30 +00:00
@ -0,0 +1,194 @@
<template>
<draggable

For a new component: can we use the sortable from vueuse?

For a new component: can we use the sortable from vueuse?
Author
Owner

Didn't know about that one, will check it out.

Would the goal here be to eventually use it everywhere instead of vuedraggable?

Didn't know about that one, will check it out. Would the goal here be to eventually use it everywhere instead of vuedraggable?
Author
Owner

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.

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.
dpschen marked this conversation as resolved
dpschen reviewed 2023-03-28 13:10:01 +00:00
@ -0,0 +65,4 @@
</ProjectSettingsDropdown>
<span class="list-setting-spacer" v-else></span>
</section>
<ProjectsNavigation

Nice!

Nice!
dpschen marked this conversation as resolved
dpschen reviewed 2023-03-28 13:12:52 +00:00
@ -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.

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?

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`?
Author
Owner

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.

> 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.
dpschen marked this conversation as resolved
dpschen reviewed 2023-03-28 13:17:48 +00:00
@ -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 use await methods directly and without onBeforeMount hook.

Can we put this component inside a `<Suspense>`? Then we can use `await` methods directly and without `onBeforeMount ` hook.
Author
Owner

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).

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

Ok. Will check
dpschen reviewed 2023-03-28 13:18:48 +00:00
@ -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.

This seems like something that the store should export instead as a computed.
Author
Owner

That makes sense.

That makes sense.
Author
Owner

Changed it.

Changed it.
dpschen marked this conversation as resolved
dpschen reviewed 2023-03-28 13:19:22 +00:00
@ -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.

This computed as well.
dpschen marked this conversation as resolved
dpschen reviewed 2023-03-28 13:24:30 +00:00
@ -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

.sort((a, b) => a.position - b.position)
Simplify to ```ts .sort((a, b) => a.position - b.position) ```
Author
Owner

Done.

Done.
dpschen marked this conversation as resolved
dpschen reviewed 2023-03-28 13:25:20 +00:00
@ -272,0 +88,4 @@
...p,
childProjects: [],
}))
.sort((a, b) => a.position < b.position ? -1 : 1))

Simplify to

.sort((a, b) => a.position - b.position)
Simplify to ```ts .sort((a, b) => a.position - b.position) ```
Author
Owner

Done.

Done.
dpschen marked this conversation as resolved
dpschen reviewed 2023-03-28 13:27:17 +00:00
@ -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?

Should we keep the old shortcut and deprecate it as long as it's not used by something else?
Author
Owner

I think it's fine to "free" it up until its used for something else without explicit deprecation.

I think it's fine to "free" it up until its used for something else without explicit deprecation.
dpschen marked this conversation as resolved
dpschen reviewed 2023-03-28 13:29:56 +00:00
@ -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 or archivedWarning

How about `archivedMessage` or `archivedWarning`
Author
Owner

I like archivedMessage, renamed it.

I like `archivedMessage`, renamed it.
dpschen marked this conversation as resolved
konrad added 1 commit 2023-03-28 13:30:30 +00:00
dpschen reviewed 2023-03-28 13:31:55 +00:00
@ -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?

If projects are archived they won't show up here automatically?
Author
Owner

yes.

yes.

We didn't include archived namespaces if I understand the code correctly.

We didn't include archived namespaces if I understand the code correctly.
Author
Owner

That's correct, so now we don't show archived projects.

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 :)

Okay. I still don't understand where the archived filterting happens now (didn't check in detail) but if you are aware all good :)
dpschen marked this conversation as resolved
konrad added 1 commit 2023-03-28 13:33:38 +00:00
continuous-integration/drone/pr Build is failing Details
39b076ef6b
chore: rename prop
dpschen reviewed 2023-03-28 13:33:42 +00:00
@ -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?

👍

👍
Author
Owner

Not sure if we should do that here in this PR - it's narrow enough to do it later.

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

I created a new issue https://kolaente.dev/vikunja/frontend/issues/3326
dpschen marked this conversation as resolved
dpschen reviewed 2023-03-28 13:35:06 +00:00
@ -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.

This seems like a relevant info. I think we shouldn't remove this feature.
Author
Owner

What do you mean?

What do you mean?

We should show the parent project here now instead.

We should show the parent project here now instead.
Author
Owner

Now doing that.

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'.

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'.
Author
Owner

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.

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.

That makes sense. Hopefully there is enough space though.
dpschen marked this conversation as resolved
konrad added 1 commit 2023-03-28 13:36:35 +00:00
dpschen reviewed 2023-03-28 13:36:57 +00:00
@ -3,3 +3,2 @@
export function getProjectTitle(l: IProject) {
if (l.id === -1) {
export function getProjectTitle(p: IProject) {

Use long var names.

Use long var names.
Author
Owner

Done.

Done.
dpschen marked this conversation as resolved
konrad added 1 commit 2023-03-28 13:37:41 +00:00
continuous-integration/drone/pr Build is failing Details
82a47f4d89
fix: simplify sort
konrad added 1 commit 2023-03-28 13:39:01 +00:00
continuous-integration/drone/pr Build is failing Details
9633e169c1
fix: use correct shortcut to open projects overview
konrad added 1 commit 2023-03-28 13:40:11 +00:00
continuous-integration/drone/pr Build is failing Details
3368a449c2
chore: rename archived message key
dpschen reviewed 2023-03-28 13:40:59 +00:00
@ -25,2 +23,4 @@
position = 0
backgroundBlurHash = ''
childProjects = []
parentProjectId = 0

Coming from the recent discussion: wouldn't it be better if this was undefined or null?

Coming from the recent discussion: wouldn't it be better if this was `undefined` or `null`?
Author
Owner

You mean to make it clear if this was not set, since it will be overridden with 0 by the api anyway?

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.

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.
konrad added 1 commit 2023-03-28 13:42:06 +00:00
continuous-integration/drone/pr Build is failing Details
92c4868511
chore: use long variable name
dpschen reviewed 2023-03-28 13:43:26 +00:00
@ -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 for userProjects. Unify?

`UserShareBaseModel` is now only used for `userProjects`. Unify?
Author
Owner

Yeah I think we should, but let's combine that with #3326

Yeah I think we should, but let's combine that with https://kolaente.dev/vikunja/frontend/issues/3326
dpschen marked this conversation as resolved
dpschen reviewed 2023-03-28 13:44:13 +00:00
@ -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?

What is a general project listing for? Aren't all projects listed in the sidebar? Why do we then still need it => added functionality?
Author
Owner

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.

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.

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.
Author
Owner

Shouldn't there be a button at the top or bottom of the list in the sidebar?

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.

> Shouldn't there be a button at the top or bottom of the list in the sidebar? 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 ListProjects ProjectsNavigation 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 with ProjectsNavigation, because why shouldn't a user be able to sort the order there as well?

**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 ~~ListProjects~~ `ProjectsNavigation` component, or at least parts of it could be used to show subprojects inside of projects. **EDIT:** Issue: https://kolaente.dev/vikunja/frontend/issues/3338 ----------- **C)** `ListProjects` should share a most of its logic with `ProjectsNavigation`, because why shouldn't a user be able to sort the order there as well?
Author
Owner

then the ListProjects view could be exactly that view.

Which one? The sidebar?

The ListProjects component, or at least parts of it could be used to show subprojects inside of projects.

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.

> then the ListProjects view could be exactly that view. Which one? The sidebar? > The ListProjects component, or at least parts of it could be used to show subprojects inside of projects. 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.

then the ListProjects view could be exactly that view.

Which one? The sidebar?

The view that 'expands to the full screen width'.

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.

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

> > then the ListProjects view could be exactly that view. > > Which one? The sidebar? The view that 'expands to the full screen width'. > 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. 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: https://kolaente.dev/vikunja/frontend/issues/3337
dpschen marked this conversation as resolved
dpschen reviewed 2023-03-28 13:44:54 +00:00
@ -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.

We should create redirects for the old routes, so that links stay intact.
dpschen reviewed 2023-03-28 13:45:56 +00:00
@ -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?

Shouldn't we load the projects here now instead?
Author
Owner

Good point, added.

Good point, added.
dpschen marked this conversation as resolved
dpschen reviewed 2023-03-28 13:46:06 +00:00
@ -116,4 +113,3 @@
async function saveFilter() {
const response = await filterService.update(filter.value)
await namespaceStore.loadNamespaces()

Same here

Same here
Author
Owner

Added.

Added.
dpschen marked this conversation as resolved
dpschen reviewed 2023-03-28 13:46:15 +00:00
@ -129,9 +125,8 @@ export function useSavedFilter(projectId?: MaybeRef<IProject['id']>) {
async function deleteFilter() {
await filterService.delete(filter.value)
await namespaceStore.loadNamespaces()

And here

And here
Author
Owner

Added

Added
dpschen marked this conversation as resolved
dpschen reviewed 2023-03-28 13:50:55 +00:00
@ -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.

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.
Author
Owner

we should replace this with an array of childProjects instead.

What's that? The projects in childProjects are just regular projects.

> we should replace this with an array of `childProjects` instead. 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.

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…

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…
Author
Owner

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 use parentProjectId? And then build the list of child projects dynamically when needed? Not sure about the performance implications here.

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 use `parentProjectId`? And then build the list of child projects dynamically when needed? Not sure about the performance implications here.

Maybe we could just ignore childProjects completely and only use parentProjectId? 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.

> Maybe we could just ignore childProjects completely and only use parentProjectId? 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.

See [normalizr](https://github.com/paularmstrong/normalizr/tree/a213bbc6e7bf328cdd46f61a3367b952dc5f30da). The utility is unmaintained but the examples are still valid. Since our store is flux inspired this applies to use as well.
Author
Owner

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.

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.

> 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. 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.
Author
Owner

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.

Makes sense, I wonder how good that would work with the dragging and dropping of projects though.

> 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. Makes sense, I wonder how good that would work with the dragging and dropping of projects though.

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.

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.

I wonder how good that would work with the dragging and dropping of projects though.

Why would that be a problem?

> 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. 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. > I wonder how good that would work with the dragging and dropping of projects though. 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.

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.
Author
Owner

Okay that means that we could simply return ids of the childProjects via a new property (e.g.) childProjectIds?

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?

That would make that step from the API obsolete.

Not really, since we need to fetch all children anyway.

> Okay that means that we could simply return ids of the childProjects via a new property (e.g.) childProjectIds? 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? > That would make that step from the API obsolete. Not really, since we need to fetch all children anyway.
Author
Owner

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.

Should that include the children of children (of children... and so on) as well?

> 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. Should that include the children of children (of children... and so on) as well?
Author
Owner

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:

Index: src/modelTypes/IProject.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/modelTypes/IProject.ts b/src/modelTypes/IProject.ts
--- a/src/modelTypes/IProject.ts	(revision 0d0b3c0ca7ac1a1894a4c49091f7b138df4f9818)
+++ b/src/modelTypes/IProject.ts	(date 1680095041514)
@@ -18,7 +18,8 @@
 	subscription: ISubscription
 	position: number
 	backgroundBlurHash: string
-	childProjects: IProject[] | null
+	// childProjects: IProject[] | null
+	childProjectIds: number[]
 	parentProjectId: number
 	
 	created: Date
Index: src/components/home/ProjectsNavigationWrapper.vue
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/components/home/ProjectsNavigationWrapper.vue b/src/components/home/ProjectsNavigationWrapper.vue
--- a/src/components/home/ProjectsNavigationWrapper.vue	(revision 0d0b3c0ca7ac1a1894a4c49091f7b138df4f9818)
+++ b/src/components/home/ProjectsNavigationWrapper.vue	(date 1680095818765)
@@ -24,7 +24,6 @@
 	.filter(p => !p.isArchived && p.isFavorite)
 	.map(p => ({
 		...p,
-		childProjects: [],
 	}))
 	.sort((a, b) => a.position - b.position))
 </script>
Index: src/components/home/ProjectsNavigation.vue
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/components/home/ProjectsNavigation.vue b/src/components/home/ProjectsNavigation.vue
--- a/src/components/home/ProjectsNavigation.vue	(revision 0d0b3c0ca7ac1a1894a4c49091f7b138df4f9818)
+++ b/src/components/home/ProjectsNavigation.vue	(date 1680095997699)
@@ -26,7 +26,7 @@
 			>
 				<section>
 					<BaseButton
-						v-if="p.childProjects.length > 0"
+						v-if="childProjects[p.id].length > 0"
 						@click="collapsedProjects[p.id] = !collapsedProjects[p.id]"
 						class="collapse-project-button"
 					>
@@ -67,7 +67,7 @@
 				</section>
 				<ProjectsNavigation
 					v-if="!collapsedProjects[p.id]"
-					v-model="p.childProjects"
+					v-model="childProjects[p.id]"
 					:can-edit-order="true"
 				/>
 			</li>
@@ -114,11 +114,15 @@
 // TODO: child projects
 const collapsedProjects = ref<{ [id: IProject['id']]: boolean }>({})
 const availableProjects = ref<IProject[]>([])
+const childProjects = ref<{ [id: IProject['id']]: boolean }>({})
 watch(
 	() => props.modelValue,
 	projects => {
 		availableProjects.value = projects
-		projects.forEach(p => collapsedProjects.value[p.id] = false)
+		projects.forEach(p => {
+			collapsedProjects.value[p.id] = false
+			childProjects.value[p.id] = projectStore.getChildProjects(p.id)
+		})
 	},
 	{immediate: true},
 )
@@ -149,8 +153,8 @@
 
 	if (project.parentProjectId !== parentProjectId && project.parentProjectId > 0) {
 		const parentProject = projectStore.getProjectById(project.parentProjectId)
-		const childProjectIndex = parentProject.childProjects.findIndex(p => p.id === project.id)
-		parentProject.childProjects.splice(childProjectIndex, 1)
+		const childProjectIndex = parentProject.childProjectIds.findIndex(pId => pId === project.id)
+		parentProject.childProjectIds.splice(childProjectIndex, 1)
 	}
 
 	try {
Index: src/stores/projects.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/stores/projects.ts b/src/stores/projects.ts
--- a/src/stores/projects.ts	(revision 0d0b3c0ca7ac1a1894a4c49091f7b138df4f9818)
+++ b/src/stores/projects.ts	(date 1680096142590)
@@ -34,6 +34,9 @@
 	const getProjectById = computed(() => {
 		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)
+	})
 
 	const findProjectByExactname = computed(() => {
 		return (name: string) => {
@@ -67,24 +70,24 @@
 		}
 
 		if (updateChildren) {
-			project.childProjects?.forEach(p => setProject(p))
+			project.childProjects?.forEach(p => setProject(new ProjectModel(p)))
 		}
 
 		// if the project is a child project, we need to also set it in the parent
 		if (project.parentProjectId) {
 			const parent = projects.value[project.parentProjectId]
 			let foundProject = false
-			parent.childProjects = parent.childProjects?.map(p => {
+			parent.childProjectIds = parent.childProjectIds?.forEach(p => {
 				if (p.id === project.id) {
 					foundProject = true
-					return project
 				}
-
-				return p
 			})
 			// If we hit this, the project now has a new parent project which it did not have before
 			if (!foundProject) {
-				parent.childProjects.push(project)
+				if (!parent.childProjectIds) {
+					parent.childProjectIds = []
+				}
+				parent.childProjectIds.push(project.id)
 			}
 			setProject(parent, false)
 		}
@@ -197,6 +200,7 @@
 		hasProjects: readonly(hasProjects),
 
 		getProjectById,
+		getChildProjects,
 		findProjectByExactname,
 		searchProject,
 
Index: src/models/project.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/models/project.ts b/src/models/project.ts
--- a/src/models/project.ts	(revision 0d0b3c0ca7ac1a1894a4c49091f7b138df4f9818)
+++ b/src/models/project.ts	(date 1680096142588)
@@ -22,7 +22,7 @@
 	subscription: ISubscription = null
 	position = 0
 	backgroundBlurHash = ''
-	childProjects = []
+	childProjectIds = []
 	parentProjectId = 0
 	
 	created: Date = null
@@ -47,7 +47,8 @@
 			this.subscription = new SubscriptionModel(this.subscription)
 		}
 		
-		this.childProjects = this.childProjects.map(p => new ProjectModel(p))
+		// debugger
+		this.childProjectIds = this.childProjects?.map(p => p.id) || []
 
 		this.created = new Date(this.created)
 		this.updated = new Date(this.updated)
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: ```patch Index: src/modelTypes/IProject.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/modelTypes/IProject.ts b/src/modelTypes/IProject.ts --- a/src/modelTypes/IProject.ts (revision 0d0b3c0ca7ac1a1894a4c49091f7b138df4f9818) +++ b/src/modelTypes/IProject.ts (date 1680095041514) @@ -18,7 +18,8 @@ subscription: ISubscription position: number backgroundBlurHash: string - childProjects: IProject[] | null + // childProjects: IProject[] | null + childProjectIds: number[] parentProjectId: number created: Date Index: src/components/home/ProjectsNavigationWrapper.vue IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/components/home/ProjectsNavigationWrapper.vue b/src/components/home/ProjectsNavigationWrapper.vue --- a/src/components/home/ProjectsNavigationWrapper.vue (revision 0d0b3c0ca7ac1a1894a4c49091f7b138df4f9818) +++ b/src/components/home/ProjectsNavigationWrapper.vue (date 1680095818765) @@ -24,7 +24,6 @@ .filter(p => !p.isArchived && p.isFavorite) .map(p => ({ ...p, - childProjects: [], })) .sort((a, b) => a.position - b.position)) </script> Index: src/components/home/ProjectsNavigation.vue IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/components/home/ProjectsNavigation.vue b/src/components/home/ProjectsNavigation.vue --- a/src/components/home/ProjectsNavigation.vue (revision 0d0b3c0ca7ac1a1894a4c49091f7b138df4f9818) +++ b/src/components/home/ProjectsNavigation.vue (date 1680095997699) @@ -26,7 +26,7 @@ > <section> <BaseButton - v-if="p.childProjects.length > 0" + v-if="childProjects[p.id].length > 0" @click="collapsedProjects[p.id] = !collapsedProjects[p.id]" class="collapse-project-button" > @@ -67,7 +67,7 @@ </section> <ProjectsNavigation v-if="!collapsedProjects[p.id]" - v-model="p.childProjects" + v-model="childProjects[p.id]" :can-edit-order="true" /> </li> @@ -114,11 +114,15 @@ // TODO: child projects const collapsedProjects = ref<{ [id: IProject['id']]: boolean }>({}) const availableProjects = ref<IProject[]>([]) +const childProjects = ref<{ [id: IProject['id']]: boolean }>({}) watch( () => props.modelValue, projects => { availableProjects.value = projects - projects.forEach(p => collapsedProjects.value[p.id] = false) + projects.forEach(p => { + collapsedProjects.value[p.id] = false + childProjects.value[p.id] = projectStore.getChildProjects(p.id) + }) }, {immediate: true}, ) @@ -149,8 +153,8 @@ if (project.parentProjectId !== parentProjectId && project.parentProjectId > 0) { const parentProject = projectStore.getProjectById(project.parentProjectId) - const childProjectIndex = parentProject.childProjects.findIndex(p => p.id === project.id) - parentProject.childProjects.splice(childProjectIndex, 1) + const childProjectIndex = parentProject.childProjectIds.findIndex(pId => pId === project.id) + parentProject.childProjectIds.splice(childProjectIndex, 1) } try { Index: src/stores/projects.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/stores/projects.ts b/src/stores/projects.ts --- a/src/stores/projects.ts (revision 0d0b3c0ca7ac1a1894a4c49091f7b138df4f9818) +++ b/src/stores/projects.ts (date 1680096142590) @@ -34,6 +34,9 @@ const getProjectById = computed(() => { 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) + }) const findProjectByExactname = computed(() => { return (name: string) => { @@ -67,24 +70,24 @@ } if (updateChildren) { - project.childProjects?.forEach(p => setProject(p)) + project.childProjects?.forEach(p => setProject(new ProjectModel(p))) } // if the project is a child project, we need to also set it in the parent if (project.parentProjectId) { const parent = projects.value[project.parentProjectId] let foundProject = false - parent.childProjects = parent.childProjects?.map(p => { + parent.childProjectIds = parent.childProjectIds?.forEach(p => { if (p.id === project.id) { foundProject = true - return project } - - return p }) // If we hit this, the project now has a new parent project which it did not have before if (!foundProject) { - parent.childProjects.push(project) + if (!parent.childProjectIds) { + parent.childProjectIds = [] + } + parent.childProjectIds.push(project.id) } setProject(parent, false) } @@ -197,6 +200,7 @@ hasProjects: readonly(hasProjects), getProjectById, + getChildProjects, findProjectByExactname, searchProject, Index: src/models/project.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/models/project.ts b/src/models/project.ts --- a/src/models/project.ts (revision 0d0b3c0ca7ac1a1894a4c49091f7b138df4f9818) +++ b/src/models/project.ts (date 1680096142588) @@ -22,7 +22,7 @@ subscription: ISubscription = null position = 0 backgroundBlurHash = '' - childProjects = [] + childProjectIds = [] parentProjectId = 0 created: Date = null @@ -47,7 +47,8 @@ this.subscription = new SubscriptionModel(this.subscription) } - this.childProjects = this.childProjects.map(p => new ProjectModel(p)) + // debugger + this.childProjectIds = this.childProjects?.map(p => p.id) || [] this.created = new Date(this.created) this.updated = new Date(this.updated)
Author
Owner

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).

I got something working! See 2557b182dde8f40f4be903e65c0485d46c5a185a There are a few cases where the performance is really bad but I didn't manage to reproduce that reliably (let alone profile it).
dpschen reviewed 2023-03-28 13:52:42 +00:00
@ -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.

See comment about having the data as sub projects from earlier.
dpschen reviewed 2023-03-28 13:53:24 +00:00
@ -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?

Unsure: shouldn't we use klona here instead?
Author
Owner

Not sure either, I just refactored this.

Not sure either, I just refactored this.

Let's use klona for this kind of stuff to be consistent.

Let's use klona for this kind of stuff to be consistent.
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen reviewed 2023-03-28 13:55:38 +00:00
@ -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.

These were meant as component local variables. If we define them now as global we probably should replace them with css variables instead.
Author
Owner

I think it's fine to use it as a scss variable since it will never change at runtime.

I think it's fine to use it as a scss variable since it will never change at runtime.
konrad marked this conversation as resolved
dpschen reviewed 2023-03-28 13:58:47 +00:00
@ -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

This component is basically a redo of a huge part of what I did with https://kolaente.dev/vikunja/frontend/pulls/2108/files
Author
Owner

Looks like it. Do you want to continue that PR? Given how it is already old and now even more outdated.

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.

I worked many hours to untangle the CSS there. I hope that we can save that effort somehow.
Author
Owner

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).

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.

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.
dpschen marked this conversation as resolved
dpschen reviewed 2023-03-28 14:01:31 +00:00
@ -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)?

This should be a computed of the store, like `hasTasks`. Shouldn't we check the length here (`projectStore.projects?.length`)?
Author
Owner

projects is an object, so checking the length of it won't work.

`projects` is an object, so checking the length of it won't work.
Author
Owner

Moved it to the store.

Moved it to the store.

Then Object.keys?.length

Then `Object.keys?.length`
Author
Owner

Then Object.keys?.length

Wouldn't that cause a call to Object.keys? I think my solution is faster.

> Then `Object.keys?.length` Wouldn't that cause a call to `Object.keys`? I think my solution is faster.
Author
Owner

Checked it: https://jsbench.me/hslfseq93y/1

Using Object.keys really is slower, and by a lot.

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.

True. Using a Map with size would probably be the perfect solution here. But that is something for another time.
konrad marked this conversation as resolved
dpschen reviewed 2023-03-28 14:04:32 +00:00
@ -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.

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.
Author
Owner
See https://kolaente.dev/vikunja/frontend/pulls/3323#issuecomment-47666
Author
Owner

It's now possible to set a parent project when creating or editing a project as well.

It's now possible to set a parent project when creating or editing a project as well.
konrad marked this conversation as resolved
dpschen reviewed 2023-03-28 14:06:02 +00:00
@ -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.

This should be replaced by a project selector in order to select the new parent project.
Author
Owner
See https://kolaente.dev/vikunja/frontend/pulls/3323#issuecomment-47666

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 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.
Author
Owner

I've now added a project select when duplicating

I've now added a project select when duplicating
konrad marked this conversation as resolved
dpschen reviewed 2023-03-28 14:07:14 +00:00
@ -14,4 +14,2 @@
ref="heading"
/>
<h6 class="subtitle" v-if="parent && parent.namespace && parent.project">
{{ getNamespaceTitle(parent.namespace) }} &rsaquo;

Shouldn't we still show the parent here?

Shouldn't we still show the parent here?
Author
Owner

I've changed it now so that is shows this:

Screenshot_20230328_174428.png

(each project is clickable individually)

For a hierarchy like this:

Screenshot_20230328_174646.png

I've changed it now so that is shows this: ![Screenshot_20230328_174428.png](/attachments/35babb55-b48a-49bf-a595-3fcc28da12dc) (each project is clickable individually) For a hierarchy like this: ![Screenshot_20230328_174646.png](/attachments/070d677e-d199-45ee-af6b-c0a6b3196681)

Can't see these images either

Can't see these images either
Author
Owner

Again 404?

Again 404?
Author
Owner

This starts to feel like a gitea bug...

This starts to feel like a gitea bug...
dpschen reviewed 2023-03-28 14:09:19 +00:00
@ -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:

await projectStore.loadProjects() // reloading the projects list so that the Favorites project shows up or is hidden when there are (or are not) favorite tasks
It seems like this should be replaced with what you wrote above: ```ts await projectStore.loadProjects() // reloading the projects list so that the Favorites project shows up or is hidden when there are (or are not) favorite tasks ```

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.

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.
Author
Owner

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.

> 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.

I've now created that method and replaced its usages.

You forgot to await the new store method

> I've now created that method and replaced its usages. You forgot to await the new store method
Author
Owner

whoops, fixed now.

whoops, fixed now.
konrad marked this conversation as resolved
konrad added 1 commit 2023-03-28 14:19:04 +00:00
dpschen added a new dependency 2023-03-28 14:24:58 +00:00
konrad added 1 commit 2023-03-28 14:27:11 +00:00
continuous-integration/drone/pr Build is failing Details
ebb239f683
feat(projects): move hasProjects check to store
Member

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.

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.
konrad added 1 commit 2023-03-28 14:31:37 +00:00
continuous-integration/drone/pr Build is failing Details
a0d45dcd89
chore(task): move toggleFavorite to store
konrad added 1 commit 2023-03-28 14:44:25 +00:00
continuous-integration/drone/pr Build is failing Details
fda2e41346
fix(filters): load projects after creating a filter
konrad added 1 commit 2023-03-28 14:44:53 +00:00
continuous-integration/drone/pr Build is failing Details
c4d13dff2c
fix(filters): load projects after deleting a filter
konrad added 1 commit 2023-03-28 14:45:11 +00:00
continuous-integration/drone/pr Build is failing Details
0405b97040
fix(filters): load projects after updating a filter
konrad added 1 commit 2023-03-28 15:25:42 +00:00
continuous-integration/drone/pr Build is failing Details
4d73a8abfc
fix: add await
Author
Owner

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

> 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
konrad added 1 commit 2023-03-28 15:48:30 +00:00
continuous-integration/drone/pr Build is failing Details
0c5589859d
feat: show all parent projects in task detail view
konrad added 1 commit 2023-03-28 16:21:26 +00:00
continuous-integration/drone/pr Build is failing Details
1322d49624
feat: show all parent projects in project search
Author
Owner

Here's a video demonstration of making a project the child of another:

vikunja-child-2023-03-28_16.37.45.webm

Here's a video demonstration of making a project the child of another: [vikunja-child-2023-03-28_16.37.45.webm](/attachments/463db5fd-4c1a-4851-ae19-ef3bf2ad62b2)
dpschen reviewed 2023-03-28 17:14:31 +00:00
@ -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?

const project = computed(() => projectStore.getProjectById(task.projectId)})

watchEffect(() => {
	baseStore.handleSetCurrentProject({
    	project: project.value
   	})
})
__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? ```ts const project = computed(() => projectStore.getProjectById(task.projectId)}) watchEffect(() => { baseStore.handleSetCurrentProject({ project: project.value }) }) ```
Author
Owner

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.

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.
konrad marked this conversation as resolved
dpschen reviewed 2023-03-28 17:18:42 +00:00
@ -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

This whole functionality is really nice for projects as well. Why not use it there => `useProjectSearch`
Author
Owner

The project store has a projectSearch method which does something similar. Or do you have something specific in mind?

The project store has a `projectSearch` method which does something similar. Or do you have something specific in mind?
dpschen reviewed 2023-03-28 17:20:59 +00:00
@ -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.

This should either be in a new helper (see the name of this file) or both should be part of the project store.
Author
Owner

Refactored it and moved to the projects store.

Refactored it and moved to the projects store.
konrad marked this conversation as resolved
dpschen reviewed 2023-03-28 17:24:44 +00:00
@ -9,0 +16,4 @@
export function getParentProjects(project: IProject, projectStore): IProject[] {
let parents = []
if (project.parentProjectId) {
if (!project.parentProjectId) {
	return [project]
}

const parentProject = projectStore.getProjectById(project.parentProjectId)

return [
	...getParentProjects(parentProject, projectStore),
	project,
]
```ts if (!project.parentProjectId) { return [project] } const parentProject = projectStore.getProjectById(project.parentProjectId) return [ ...getParentProjects(parentProject, projectStore), project, ] ```
konrad marked this conversation as resolved
konrad added 1 commit 2023-03-28 19:28:42 +00:00
konrad added 1 commit 2023-03-28 19:31:44 +00:00
continuous-integration/drone/pr Build is failing Details
0d0b3c0ca7
fix: make computed side-effect free
konrad added 1 commit 2023-03-29 14:09:29 +00:00
konrad added 1 commit 2023-03-29 14:20:03 +00:00
konrad added 1 commit 2023-03-29 15:05:39 +00:00
konrad added 1 commit 2023-03-29 15:10:25 +00:00
konrad added 1 commit 2023-03-29 15:13:03 +00:00
continuous-integration/drone/pr Build is failing Details
1f69763c3d
fix: don't show > for top-level projects
dpschen reviewed 2023-03-29 15:56:15 +00:00
dpschen left a comment
Member

Some more stuff that I found. Will need more rounds.

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?

Loading all projects in between is not necessary anymore?
Author
Owner

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.

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.

Non archived root projects seems like something that the store should provide as computed.
Author
Owner

Done

Done
konrad marked this conversation as resolved
@ -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 of modelValue for ProjectsNavigation. I guess that when the template gets rendered it wants to bind to something like @update:modelValue="(newVal) => projects.value = newVal"

Probably because you use `v-model` instead of `modelValue` for ProjectsNavigation. I guess that when the template gets rendered it wants to bind to something like `@update:modelValue="(newVal) => projects.value = newVal"`
Author
Owner

I've now changed it to :modelValue and that seems to work.

I've now changed it to `:modelValue` and that seems to work.
konrad marked this conversation as resolved
@ -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.

`favoriteProjects` also seems like something a store should export.
Author
Owner

Done

Done
konrad marked this conversation as resolved
@ -0,0 +27,4 @@
})
const favoriteProjects = computed(() => projectStore.projectsArray
.filter(p => !p.isArchived && p.isFavorite)
.map(p => ({

Why the mapping / returning of a copy?

Why the mapping / returning of a copy?
Author
Owner

That was a relic, I've removed it now.

That was a relic, I've removed it now.
konrad marked this conversation as resolved
@ -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

This class doesn't isn't used in the template
Author
Owner

I've renamed it - it is coming from the Loader component.

I've renamed it - it is coming from the `Loader` component.
konrad marked this conversation as resolved
konrad added 3 commits 2023-04-01 09:15:37 +00:00
konrad added 1 commit 2023-04-01 09:18:15 +00:00
continuous-integration/drone/pr Build is failing Details
08e2a44b20
chore: export favorite projects from store
konrad added 1 commit 2023-04-01 09:20:33 +00:00
continuous-integration/drone/pr Build is failing Details
9fd6ea2f4d
chore: move loader class
konrad added 1 commit 2023-04-01 09:23:13 +00:00
continuous-integration/drone/pr Build is failing Details
2cf1e91b63
fix: passing readonly projects data to navigation
konrad force-pushed feature/projects-all-the-way-down from 2cf1e91b63 to 733f0c1e19 2023-04-01 09:24:31 +00:00 Compare
konrad added 1 commit 2023-04-01 13:20:05 +00:00
continuous-integration/drone/pr Build is failing Details
33b8a96bb8
chore: use klona to clone project objet
dpschen requested changes 2023-04-01 13:20:20 +00:00
@ -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?

Didn't we just remove this condition, so that also one can also adjust settings of favorited lists?
Author
Owner

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.

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.

My bad I confused favorite projects with the 'Favorite' project.
dpschen marked this conversation as resolved
@ -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.

Also don't render if there are no child projects.
Author
Owner

But then it won't be possible to drag a project "under" a parent to make it a child of that parent.

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?

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?
Author
Owner

Yes, .menu is a class from bulma.

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?

You mean adding the class into the styles definition of the ProjectsNavigation component?

Yes, `.menu` is a class from bulma. > 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? 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

Use `components/misc/loading.vue`
Author
Owner

That was an old test I did, I've now removed it.

That was an old test I did, I've now removed it.
dpschen marked this conversation as resolved
@ -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 the list-item, because there are no added semantics. So listing the links is ok.

Something that I don't know that long myself: adding a `<ul>` inside a `<nav>` doesn't give added benefit if we hide the `list-item`, because there are no added semantics. So listing the links is ok.
Author
Owner

So we could get rid of the ul entierly and only add the links?

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).

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.

If Bulma doesn't require you to use `<ul>` use `<menu>` instead.

menu still missing here. (you changed it in ProjectsNavigation).

`menu` still missing here. (you changed it in ProjectsNavigation).
Author
Owner

Done

Done
dpschen marked this conversation as resolved
@ -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.

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.

`menu-label` isn't used anymore.
Author
Owner

Removed.

Removed.
dpschen marked this conversation as resolved
konrad added 1 commit 2023-04-01 14:48:19 +00:00
continuous-integration/drone/pr Build is failing Details
5027ff460c
fix: remove leftover suspense
konrad added 1 commit 2023-04-01 14:54:28 +00:00
continuous-integration/drone/pr Build is failing Details
5367573ddc
chore: remove unused class
dpschen reviewed 2023-04-01 19:02:38 +00:00
@ -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.

Rename to `loadProjectArray` to make the vars better distinguishable.
dpschen marked this conversation as resolved
@ -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?

Should we add a test for projects created from the sidebar?
Author
Owner

But you can't create a project from the sidebar?

But you can't create a project from the sidebar?

it was possible in the contextmenu of namespaces

it was possible in the contextmenu of namespaces
Author
Owner

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.

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).

Use [`<menu>`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/menu) (now I remembered the correct element).
Author
Owner

Done

Done
dpschen marked this conversation as resolved
@ -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 via element.

Use long var name. I thought for a bit that for `<draggable>` you can define the type of dom node of the child item via `element`.
Author
Owner

Done.

Done.
dpschen marked this conversation as resolved
@ -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!)

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 with childProjects[p.id] because we can pass only the project.

Move this whole block in a new `ProjectNavigationItem.vue` component. Reduces also the whole complexity with `childProjects[p.id]` because we can pass only the project.
Author
Owner

Move this whole block in a new ProjectNavigationItem.vue component. Reduces also the whole complexity with childProjects[p.id] because we can pass only the project.

Done

> Move this whole block in a new ProjectNavigationItem.vue component. Reduces also the whole complexity with childProjects[p.id] because we can pass only the project. Done
Author
Owner

Shouldn't a nav hold multiple navigation items?

Shouldn't a `nav` hold multiple navigation items?

Shouldn't a nav hold multiple navigation items?

Yes! Sry I misread the position, where the <section> is.

Done

Okay you moved now only the item without the list below inside.
What i meant was:

  • Move the complete <li> inside ProjectsNavigationItem.vue.
  • ProjectsNavigation.vue is then used inside ProjectsNavigationItem

This whole block can then be simplified:

const collapsedProjects = ref<{ [id: IProject['id']]: boolean }>({})
const availableProjects = ref<IProject[]>([])
const childProjects = ref<{ [id: IProject['id']]: boolean }>({})
watch(
	() => props.modelValue,
	projects => {
		availableProjects.value = projects || []
		projects?.forEach(p => {
			collapsedProjects.value[p.id] = false
			childProjects.value[p.id] = projectStore.getChildProjects(p.id)
				.sort((a, b) => a.position - b.position)
		})
	},
	{immediate: true},
)

Because we can save the collapsed state inside each item we don't need to manage a list anymore.

const childProjectsOpen = ref(true)

// if getChildProjects returns the list sorted by position by default we wouldn't even need this computed
const childProjects = computed(() => {
	const projects = projectStore.getChildProjects(p.id)
	return projects.sort((a, b) => a.position - b.position)
})
> Shouldn't a `nav` hold multiple navigation items? Yes! Sry I misread the position, where the `<section>` is. > Done Okay you moved now only the item without the list below inside. What i meant was: - Move the complete `<li>` inside `ProjectsNavigationItem.vue`. - `ProjectsNavigation.vue` is then used inside ProjectsNavigationItem This whole block can then be simplified: ```ts const collapsedProjects = ref<{ [id: IProject['id']]: boolean }>({}) const availableProjects = ref<IProject[]>([]) const childProjects = ref<{ [id: IProject['id']]: boolean }>({}) watch( () => props.modelValue, projects => { availableProjects.value = projects || [] projects?.forEach(p => { collapsedProjects.value[p.id] = false childProjects.value[p.id] = projectStore.getChildProjects(p.id) .sort((a, b) => a.position - b.position) }) }, {immediate: true}, ) ``` Because we can save the collapsed state inside each item we don't need to manage a list anymore. ```ts const childProjectsOpen = ref(true) // if getChildProjects returns the list sorted by position by default we wouldn't even need this computed const childProjects = computed(() => { const projects = projectStore.getChildProjects(p.id) return projects.sort((a, b) => a.position - b.position) }) ```

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. The ProjectsNavigation component would be the last child insie ProjectsNavigationItem

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. The `ProjectsNavigation` component would be the last child insie `ProjectsNavigationItem`
Author
Owner

That makes sense. I've moved most of the logic over, as you suggested.

So we don't even need the <section> then and can instead use the <li>.

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 the li 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.

That makes sense. I've moved most of the logic over, as you suggested. > So we don't even need the `<section>` then and can instead use the `<li>`. 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 the `li` 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?

What consequence has this?
Author
Owner

This now works, it didn't in an earlier version. I've removed the comment.

This now works, it didn't in an earlier version. I've removed the comment.
dpschen marked this conversation as resolved
@ -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?

What exactly needs to be done for child projects?
Author
Owner

That was an old comment, I've removed it

That was an old comment, I've removed it
dpschen marked this conversation as resolved
@ -0,0 +15,4 @@
const projectStore = useProjectStore()
await projectStore.loadProjects()

Call this in App.vue and add a isLoadingPromise that contains the returned promise of the loadProjects function to the projectStore which we then can await here instead.

Call this in `App.vue` and add a `isLoadingPromise` that contains the returned promise of the `loadProjects` function to the projectStore which we then can await here instead.
Author
Owner

What would be the advantage of this? I feel like this would be a lot more complicated.

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.

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](https://kolaente.dev/vikunja/frontend/src/commit/74d688b8d20838b2e0dbbe47e04ae0305e48ec6e/src/components/home/contentAuth.vue#L117-L118).
Author
Owner

I've now moved it to contentAuth but am using the isLoading property of the project store to show the loading state. That means I can't use the Suspense component, but the loading property already exists and is populated, might as well use it.

I've now moved it to `contentAuth` but am using the `isLoading` property of the project store to show the loading state. That means I can't use the `Suspense` 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 the Suspense, right?

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 the `Suspense`, right?
Author
Owner

Yes, I created it only to use it with Suspense. Should I include its contents back into navigation?

Yes, I created it only to use it with `Suspense`. Should I include its contents back into `navigation`?

I think that would be better now. Sry for the back and forth.

I think that would be better now. Sry for the back and forth.
Author
Owner

No worries, changed it back.

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:

.menu + .menu {
	margin-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: ```scss .menu + .menu { margin-top: math.div($navbar-padding, 2); } ```
Author
Owner

Done.

Done.
dpschen marked this conversation as resolved
@ -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.

This changes styles inside the component. If the styles need to be adjusted the component should be changed instead.
Author
Owner

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?

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.

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.
Author
Owner

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.

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.
dpschen marked this conversation as resolved
@ -153,9 +140,6 @@ async function unsubscribe() {
let message = ''
switch (props.entity) {

Add type to props definition: Should be ISubscription['entity']

Add type to props definition: Should be `ISubscription['entity']`
Author
Owner

Done.

Done.
dpschen marked this conversation as resolved
@ -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

Remove added indention
Author
Owner

Done.

Done.
dpschen marked this conversation as resolved
konrad added 3 commits 2023-04-01 19:47:35 +00:00
konrad added 2 commits 2023-04-01 19:52:58 +00:00
konrad added 1 commit 2023-04-01 19:56:47 +00:00
continuous-integration/drone/pr Build is failing Details
0449242f87
chore: improve prop type definition
konrad added 1 commit 2023-04-01 19:57:39 +00:00
continuous-integration/drone/pr Build is failing Details
f451f69dc6
fix: indention
konrad added 1 commit 2023-04-01 20:02:12 +00:00
continuous-integration/drone/pr Build is failing Details
ad4eb25eac
chore: use <menu> instead of <ul>
konrad added 1 commit 2023-04-01 20:03:58 +00:00
continuous-integration/drone/pr Build is failing Details
24420cb14e
chore: remove old comment
konrad added 1 commit 2023-04-02 12:44:18 +00:00
konrad added 1 commit 2023-04-02 12:52:34 +00:00
konrad added 1 commit 2023-04-02 15:21:14 +00:00
konrad added 1 commit 2023-04-02 17:51:51 +00:00
continuous-integration/drone/pr Build is failing Details
1889abfa9b
chore: move more logic to ProjectsNavigationItem.vue
konrad added 2 commits 2023-04-03 09:19:53 +00:00
konrad added 1 commit 2023-04-03 09:28:05 +00:00
continuous-integration/drone/pr Build is failing Details
4868661dc4
feat: don't use child_projects property from api
konrad added 1 commit 2023-04-03 09:29:14 +00:00
continuous-integration/drone/pr Build is failing Details
4d5d1b3e31
fix: collapsing child projects
konrad added 1 commit 2023-04-03 10:10:03 +00:00
continuous-integration/drone/pr Build is failing Details
154d85bd3f
fix: use menu tag everywhere
konrad added 1 commit 2023-04-03 11:53:34 +00:00
continuous-integration/drone/pr Build is failing Details
d694f168b2
feat: add setting for infinite nesting
Author
Owner

I've added a nesting limit.

I've added a nesting limit.
dpschen reviewed 2023-04-10 18:23:10 +00:00
@ -0,0 +45,4 @@
canEditOrder: boolean,
canCollapse?: boolean,
}>()
const emit = defineEmits(['update:modelValue'])

Add types for emit

Add types for emit
Author
Owner

Done

Done
konrad marked this conversation as resolved
@ -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

These options should either contain all dragOptions or be defined inline
Author
Owner

Moved it all inline.

Moved it all inline.
konrad marked this conversation as resolved
@ -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 of v-model for the draggable?

Is this even necessary if we use `modelValue` instead of `v-model` for the draggable?
Author
Owner

v-model is required, using modelValue for the draggable component does not work.

`v-model` is required, using `modelValue` 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.

Set from outside, since this id is related to the sorting.
Author
Owner

Done

Done
dpschen marked this conversation as resolved
@ -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.

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.
Author
Owner

Done

Done
dpschen marked this conversation as resolved
@ -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

Fix indention

Pass 'handle class name from parent => separate concerns / source of truth

Pass 'handle class name from parent => separate concerns / source of truth
Author
Owner

But the indention is correct?

But the indention is correct?
Author
Owner

Pass 'handle class name from parent => separate concerns / source of truth

Can you explain that a little more?

> Pass 'handle class name from parent => separate concerns / source of truth 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.

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.

Use `Expandable` component for this.
Author
Owner

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.-

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 the open prop) or static (not rendering the Expandable at all via v-if).

I created an example how to use this in https://kolaente.dev/vikunja/frontend/commit/51e29af010defc5f6c46f85dbb7311904a7d40e1. I was unsure which parts parts should be dynamically be filled (via the `open` prop) or static (not rendering the `Expandable` at all via `v-if`).
@ -142,3 +51,1 @@
</draggable>
</template>
</nav>
<Loading variant="small" v-if="projectsLoading"/>

Move v-if to front

Move `v-if` to front
Author
Owner

Done

Done
dpschen marked this conversation as resolved
@ -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.

Remove both computed above and use store + property directly instead.
Author
Owner

Done

Done
dpschen marked this conversation as resolved
@ -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

Missing space
Author
Owner

Done

Done
dpschen marked this conversation as resolved
konrad added 6 commits 2023-04-10 18:59:18 +00:00
Member

Can't rename a project via

Contextmenu => Edit => new name => save

image

Can't rename a project via Contextmenu => Edit => new name => save ![image](/attachments/654fda71-b854-412b-aeff-f25104dc3f7a)
126 KiB
Member
  • 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 Inbox the 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":
    image

  • Not enough spacing to button inside /projects:
    image

  • 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:
    image
    old

    image
    new

- [x] Inbox shouldn't be able to have children. Seems very weird that this is possible => *For now we keep the current implementation* - [x] 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* - [x] Inbox should always be on top. => *For now we keep the current implementation* - [ ] It shouldn't be possible to make ~~Inbox~~ *the 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":* ![image](/attachments/95809371-de68-46fb-8e0d-f904775e7784) - [ ] Not enough spacing to button inside `/projects`: ![image](/attachments/a9307920-5978-4041-848f-cfaa3ebfd4d1) - [ ] 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: ![image](/attachments/1f9bbdd7-238f-45b8-9a0f-0306e48c2ffd) *old* ![image](/attachments/7d01bd4c-fb3a-4e57-bbad-25f27d74bc29) *new*
dpschen reviewed 2023-04-11 15:02:57 +00:00
Dockerfile Outdated
@ -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

Add types to `env.d.ts`
Author
Owner

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...

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...
dpschen reviewed 2023-04-11 15:06:45 +00:00
@ -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 or changeOrder.

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` or `changeOrder`.
Author
Owner

I was able to move the whole thing into the updateProject method of the store.

I was able to move the whole thing into the `updateProject` method of the store.
dpschen reviewed 2023-04-11 15:08:09 +00:00
@ -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.

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.
Author
Owner

It looks like it works fine without the emit. We're always binding with :modelValue instead of v-model anyway…

It looks like it works fine without the emit. We're always binding with `:modelValue` instead of `v-model` anyway…
dpschen reviewed 2023-04-11 15:09:37 +00:00
@ -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 be undefined.
Could it be that ts doesn't display errors in the editor for you?

Define default types or handle undefined defaults. E.g. `level` might be `undefined`. Could it be that ts doesn't display errors in the editor for you?
Author
Owner

Added.

Could it be that ts doesn't display errors in the editor for you?

Looks like it 🤔

Added. > Could it be that ts doesn't display errors in the editor for you? Looks like it 🤔
dpschen reviewed 2023-04-11 15:16:53 +00:00
@ -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]. Since projects is exported we should replace uses of this computed. We might need to create new simple computeds where used. Depending on usecase something like

const myProject = computed(() => projects.value[myProjectId.value])
This computed seems really unnecessary. Reason: We can achieve the same (and faster) by using: `projects.value[id]`. Since `projects` is exported we should replace uses of this computed. We might need to create new simple computeds where used. Depending on usecase something like ```ts const myProject = computed(() => projects.value[myProjectId.value]) ```
Author
Owner

We'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.

We'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.
dpschen reviewed 2023-04-11 15:18:55 +00:00
@ -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 fallback is unnecessary OR the type of the projects ref is wrong. Because `Object.values` on an object should always return an array.
Author
Owner

The type is correct. I've removed the fallback.

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.

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.
Author
Owner

Should be fixed now.

Should be fixed now.
konrad marked this conversation as resolved
dpschen reviewed 2023-04-11 15:21:40 +00:00
@ -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?

This changes project before calling update. was that intended? What happens if `parentProject.value.id` is undefined?
Author
Owner

It was not intended. I've changed it now so that it checks it before and provides a proper fallback.

It was not intended. I've changed it now so that it checks it before and provides a proper fallback.
dpschen reviewed 2023-04-11 15:23:40 +00:00
@ -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.

The three lines above should be called via a deep watcher on the current project. Not as a side effect.
Author
Owner

You mean setting the current project in the base store?

You mean setting the current project in the base store?

I mean:

  1. remove this sideeffect of the setProject function:
if (baseStore.currentProject?.id === project.id) {
	baseStore.setCurrentProject(project)
}
  1. Instead add a watcher on the project that has the id of the current project.
watchEffect(() => baseStore.setCurrentProject(projects.value[baseStore.currentProject.id])
)

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 mean: 1) remove this sideeffect of the `setProject` function: ``` if (baseStore.currentProject?.id === project.id) { baseStore.setCurrentProject(project) } ``` 2) Instead add a watcher on the project that has the id of the current project. ```ts watchEffect(() => baseStore.setCurrentProject(projects.value[baseStore.currentProject.id]) ) ``` 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)
Author
Owner

I've now changed it to use a watcher.

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)

That sounds like a good idea, but let's push that to another PR.

I've now changed it to use a watcher. > 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) That sounds like a good idea, but let's push that to another PR.
Author
Owner

I've now changed it to use a watcher.

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)

That sounds like a good idea, but let's push that to another PR.

I've now changed it to use a watcher. > 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) That sounds like a good idea, but let's push that to another PR.
dpschen reviewed 2023-04-11 15:28:24 +00:00
@ -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:

computed(() => Boolean(projectsArray.value.length))
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: ```ts computed(() => Boolean(projectsArray.value.length)) ```
Author
Owner

Fixed.

Fixed.
dpschen reviewed 2023-04-11 15:32:21 +00:00
@ -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?

If I set an id for `parentID` in the passed project: will this update the parents childProjects?
Author
Owner

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.

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.
dpschen reviewed 2023-04-11 15:34:08 +00:00
@ -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?

- 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?
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen reviewed 2023-04-11 15:38:19 +00:00
@ -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.

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.
Author
Owner

But setProjects does not return anything? Neither does setProject.

These methods don't modify anything. I think it's fine to leave it like it is.

But `setProjects` does not return anything? Neither does `setProject`. These methods don't modify anything. I think it's fine to leave it like it is.
dpschen reviewed 2023-04-11 15:39:35 +00:00
@ -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?

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?
Author
Owner

I like that. Changed it.

I like that. Changed it.
dpschen reviewed 2023-04-11 16:01:52 +00:00
@ -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?

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?
Author
Owner

I've moved it out of the composable. It's only used in one place anyway...

I've moved it out of the composable. It's only used in one place anyway...
dpschen reviewed 2023-04-11 16:04:46 +00:00
@ -10,2 +10,3 @@
@import 'helpers';
@import 'helpers';
@import 'navigation';

Since these styles are only used in the navigation component: import them only there

Since these styles are only used in the navigation component: import them only there
Author
Owner

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)

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)
dpschen reviewed 2023-04-11 16:07:26 +00:00
@ -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?

Why did we remove this button?
Author
Owner

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.

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.
dpschen reviewed 2023-04-11 16:08:22 +00:00
@ -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 directly

Don't wrap computed in computed. use `projectStore.hasProjects` directly
Author
Owner

Done

Done
konrad marked this conversation as resolved
dpschen reviewed 2023-04-11 16:12:17 +00:00
@ -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?

Why is this commented out?
Author
Owner

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).

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).
dpschen reviewed 2023-04-11 16:14:44 +00:00
@ -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.

Create a new exported computed from the store `archivedProjects`. We can switch between the normal projectsArray and the archived computed when returning here.
Author
Owner

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.

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)?

const projects = computed(() => showArchived.value
	? projectStore.projectsArray
	: projectStore.projectsArray.filter(({isArchived}) => !isArchived)
})

This way we don't have to filter it if we don't need to.

Isn't that what `projectsArray` already is (list of archived and unarchived projects)? ```ts const projects = computed(() => showArchived.value ? projectStore.projectsArray : projectStore.projectsArray.filter(({isArchived}) => !isArchived) }) ``` This way we don't have to filter it if we don't need to.
Author
Owner

Good catch, I've now changed it.

Good catch, I've now changed it.
dpschen reviewed 2023-04-11 16:17:18 +00:00
@ -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.

Use a computed getter / setter for parent project via `project.id`.
Author
Owner

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.

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)

See this comment: https://kolaente.dev/vikunja/frontend/pulls/3323#issuecomment-49061

Main point is: source of truth should be url. And url will set parentId inside the project reactive. The parentProject 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 the project 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)

Main point is: source of truth should be url. And url will set parentId inside the `project` reactive. The `parentProject` 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 the `project` 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 https://kolaente.dev/vikunja/frontend/pulls/3323#issuecomment-48860
dpschen reviewed 2023-04-11 16:18:05 +00:00
@ -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.

Add immediate watcher from route. Watch for parentProjectId.
Author
Owner

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.

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.

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.

> 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.
Author
Owner

I've now added this. It may not be the most optimal version but I think it's good enough.

I've now added this. It may not be the most optimal version but I think it's good enough.
dpschen reviewed 2023-04-11 16:18:36 +00:00
@ -63,2 +72,4 @@
}
showError.value = false
if (parentProject.value) {

If we have the computed getter / setter this shouldn't be necessary

If we have the computed getter / setter this shouldn't be necessary
dpschen reviewed 2023-04-11 16:19:35 +00:00
@ -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.

Push to new project route in store.
Author
Owner

Done.

Done.
dpschen reviewed 2023-04-11 16:23:32 +00:00
@ -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 add duplicateProject method.

Use `useProject` to get current project here and add `duplicateProject` method.
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen reviewed 2023-04-11 16:24:25 +00:00
@ -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.

Use computed getter / setter to set parentProjectId of project instead.
Author
Owner

I've changed this since moving the parent project logic out of the composable.

I've changed this since moving the parent project logic out of the composable.
konrad marked this conversation as resolved
dpschen reviewed 2023-04-11 16:27:58 +00:00
@ -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 computed parentProjects that automatically updates instead?

This won't update dynamically. Should we change the `getParentProjects` to a computed `parentProjects` that automatically updates instead?
Author
Owner

I tried to change it but it fails with getAncestors is not a function. Any idea?

I tried to change it but it fails with `getAncestors is not a function`. Any idea?
Author
Owner

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.

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.
dpschen reviewed 2023-04-11 16:29:46 +00:00
@ -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"> &gt; </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.

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.
Author
Owner

But that's what this does?

But that's what this does?
dpschen reviewed 2023-04-11 16:32:49 +00:00
@ -548,3 +539,1 @@
}
return namespaceStore.getProjectAndNamespaceById(task.projectId)
const project = computed(() => task.projectId ? projectStore.getProjectById(task.projectId) : null)

getProjectById returns undefined if there is no project with that id. So why not use that directly?

`getProjectById` returns `undefined` if there is no project with that id. So why not use that directly?
Author
Owner

Done

Done
konrad marked this conversation as resolved
dpschen reviewed 2023-04-11 16:37:04 +00:00
@ -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?

If we receive child projects like this from the server: Do we have that info somewhere else?
Author
Owner

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.

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.
dpschen reviewed 2023-04-11 16:38:02 +00:00
@ -6,3 +5,3 @@
projectId: number
namespaceId: INamespace['id']
project: IProject
parentProjectId: number

IProject['id']

`IProject['id']`
Author
Owner

Done.

Done.
dpschen marked this conversation as resolved
dpschen reviewed 2023-04-11 16:38:53 +00:00
src/main.ts Outdated
@ -23,6 +23,7 @@ declare global {
API_URL: string;
SENTRY_ENABLED: boolean;
SENTRY_DSN: string;
INFINITE_PROJECT_NESTING_ENABLED: boolean;

PROJECT_INFINITE_NESTING_ENABLED

`PROJECT_INFINITE_NESTING_ENABLED`
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
Author
Owner

Can't rename a project via

Contextmenu => Edit => new name => save

image

I was not able to reproduce this.

> Can't rename a project via > > Contextmenu => Edit => new name => save > > ![image](/attachments/654fda71-b854-412b-aeff-f25104dc3f7a) I was not able to reproduce this.
konrad added 3 commits 2023-04-12 08:35:25 +00:00
Author
Owner

Inbox shouldn't be able to have children. Seems very weird that this is possible

Inbox should always be on top.

It shouldn't be possible to make Inbox a child of another project.

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…

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.

That's the kind of additional complexity I meant…

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.

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.

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.

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?

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":¦

I noticed that, but couldn't figure out a solution. I like the idea, changed it. Let me know what you think.

Not enough spacing to button inside /projects:

Fixed.

restoring an archived subproject shouldn't be possible, because it destroys the parent child relationshop. Possible alternative: only make parent restorable.

You mean if the parent is archived?

active project is missing blue line on side indicating that it's currently open

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.

size of clicking area got reduced: left of handle isn't clickable anymore. Easy to see if you compare the focus state:

Fixed.

> Inbox shouldn't be able to have children. Seems very weird that this is possible > Inbox should always be on top. > It shouldn't be possible to make Inbox a child of another project. 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… > 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. That's the kind of additional complexity I meant… > 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. 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. > 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. 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? > 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":¦ I noticed that, but couldn't figure out a solution. I like the idea, changed it. Let me know what you think. > Not enough spacing to button inside /projects: Fixed. > restoring an archived subproject shouldn't be possible, because it destroys the parent child relationshop. Possible alternative: only make parent restorable. You mean if the parent is archived? > active project is missing blue line on side indicating that it's currently open 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. > size of clicking area got reduced: left of handle isn't clickable anymore. Easy to see if you compare the focus state: Fixed.
konrad added 1 commit 2023-04-12 08:39:15 +00:00
continuous-integration/drone/pr Build is failing Details
9cce810b91
chore: rename flag
konrad added 1 commit 2023-04-12 08:46:01 +00:00
continuous-integration/drone/pr Build is failing Details
d4faa06074
fix: move parent project child id mutation to store
konrad added 2 commits 2023-04-12 08:51:45 +00:00
konrad added 1 commit 2023-04-12 09:14:00 +00:00
continuous-integration/drone/pr Build is failing Details
cb122a02eb
fix: remove getProjectById and replace all usages of it
konrad added 1 commit 2023-04-12 09:16:00 +00:00
continuous-integration/drone/pr Build is failing Details
13734848cf
fix: remove unnecessary fallback
konrad added 1 commit 2023-04-12 09:27:49 +00:00
continuous-integration/drone/pr Build is failing Details
c576dfe0d5
fix: recreate project instead of editing before
konrad added 1 commit 2023-04-12 09:31:20 +00:00
continuous-integration/drone/pr Build is failing Details
1958f5a51d
fix: properly determine if there are projects
konrad added 1 commit 2023-04-12 09:37:13 +00:00
continuous-integration/drone/pr Build is failing Details
335cbb74db
fix: remove leftovers of childIds
konrad added 1 commit 2023-04-12 09:38:56 +00:00
continuous-integration/drone/pr Build is failing Details
f37ae218c1
fix: return updated project instead of the old one
konrad added 2 commits 2023-04-12 09:48:44 +00:00
konrad added 1 commit 2023-04-12 09:56:19 +00:00
continuous-integration/drone/pr Build is failing Details
6987231c9b
chore: don't wrap a computed in another computed
konrad added 1 commit 2023-04-12 09:57:02 +00:00
continuous-integration/drone/pr Build is failing Details
4988379500
chore: remove unused code
konrad added 1 commit 2023-04-12 10:07:55 +00:00
konrad added 1 commit 2023-04-12 10:18:17 +00:00
continuous-integration/drone/pr Build is failing Details
f705d2366c
chore: move duplicate project logic to composable
konrad added 1 commit 2023-04-12 10:31:28 +00:00
konrad added 1 commit 2023-04-12 10:35:03 +00:00
continuous-integration/drone/pr Build is failing Details
467de38e20
chore: use project id type
Member

To avoid the complexity, I'd like to avoid making it special, since that would mean more complexity in the api as well…

Yes that makes sense.

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.

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.

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.

Might be that I had an older version

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?

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.

You mean if the parent is archived?

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 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.

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.

size of clicking area got reduced: left of handle isn't clickable anymore. Easy to see if you compare the focus state:

Fixed.

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.

> To avoid the complexity, I'd like to avoid making it special, since that would mean more complexity in the api as well… Yes that makes sense. > 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. 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. > 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. Might be that I had an older version > 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? 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. > You mean if the parent is archived? 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 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. 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. > > size of clicking area got reduced: left of handle isn't clickable anymore. Easy to see if you compare the focus state: > > Fixed. 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.
dpschen reviewed 2023-04-12 14:56:16 +00:00
@ -0,0 +91,4 @@
.sort((a, b) => a.position - b.position)
})
const canNestDeeper = computed(() => {

Simplify:

const canNestDeeper = computed(() => props.level >= 2 && window.PROJECT_INFINITE_NESTING_ENABLED)
Simplify: ```ts const canNestDeeper = computed(() => props.level >= 2 && window.PROJECT_INFINITE_NESTING_ENABLED) ```
Author
Owner

But that would return false for the first two levels?

But that would return `false` for the first two levels?
dpschen reviewed 2023-04-12 14:58:49 +00:00
@ -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:

[Vue warn] Set operation on key "0" failed: target is readonly. [...]

This sorts the returned computed of the store => sort already in store OR create copy (worse performance) Error in vue developer tools: > [Vue warn] Set operation on key "0" failed: target is readonly. [...]
Author
Owner

Now sorting in store, that seems to work (or at least there are no errors now)

Now sorting in store, that seems to work (or at least there are no errors now)
dpschen reviewed 2023-04-12 14:59:00 +00:00
@ -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:

[Vue warn] Set operation on key "0" failed: target is readonly. [...]

This sorts the returned computed of the store => sort already in store OR create copy (worse performance) Error in vue developer tools: > [Vue warn] Set operation on key "0" failed: target is readonly. [...]
Author
Owner

Fixed as well.

Fixed as well.
Member
  • It should not be possible to delete a project that is currently marked as default.
- [ ] It should not be possible to delete a project that is currently marked as default.
dpschen reviewed 2023-04-12 15:32:16 +00:00
@ -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

Hide field if there are no other projects. Maybe show note instead
Author
Owner

Now hiding it.

Now hiding it.
dpschen reviewed 2023-04-12 15:46:41 +00:00
@ -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!

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!
Author
Owner

Re-added the space to the logo

Re-added the space to the logo
konrad marked this conversation as resolved
konrad added 1 commit 2023-04-14 15:06:24 +00:00
continuous-integration/drone/pr Build is failing Details
c2e9a11d52
feat: replace color dot with handle icon on hover
Author
Owner

How about indicating somehow that the inbox is the current default project? Afaik this is not visible anywhere other than the settings.

Where would you indicate that? I don't think we should add yet another icon to the sidebar but maybe in the projects overview?

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.

Done that now.

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).

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:

image

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.

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.

> How about indicating somehow that the inbox is the current default project? Afaik this is not visible anywhere other than the settings. Where would you indicate that? I don't think we should add yet another icon to the sidebar but maybe in the projects overview? > 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. Done that now. > 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). 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: ![image](/attachments/682e50b6-1735-4b78-a049-55bf55b8ce77) > 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. 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.
4.1 KiB
Author
Owner

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 tried that, but it did not work as expected:

image

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.

> 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 tried that, but it did not work as expected: ![image](/attachments/c9e8c4ab-f11d-4120-aac4-b4a75c116be7) 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.
8.2 KiB
konrad added 1 commit 2023-04-14 15:21:33 +00:00
continuous-integration/drone/pr Build is failing Details
d27ce77c95
fix: sort in store
konrad added 1 commit 2023-04-14 15:25:14 +00:00
continuous-integration/drone/pr Build is failing Details
ffe9d93a62
chore: re-add top menu spacing
konrad added 1 commit 2023-04-14 15:26:55 +00:00
konrad added 1 commit 2023-04-14 15:41:15 +00:00
konrad added 1 commit 2023-04-14 15:43:07 +00:00
continuous-integration/drone/pr Build is failing Details
aefdefcf38
chore: remove type annotation for computed
Author
Owner
  • It should not be possible to delete a project that is currently marked as default.

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.

> - [ ] It should not be possible to delete a project that is currently marked as default. 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.
konrad added 1 commit 2023-04-14 15:55:51 +00:00
continuous-integration/drone/pr Build is failing Details
a7bb5d8564
chore: don't recalculate everything
konrad added 1 commit 2023-04-14 16:18:10 +00:00
konrad added 1 commit 2023-04-18 09:39:48 +00:00
continuous-integration/drone/pr Build is failing Details
1731969898
fix: lint
Member

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!

Beep boop, I'm a bot.

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! > Beep boop, I'm a bot.
konrad force-pushed feature/projects-all-the-way-down from 1731969898 to 9241c7e587 2023-04-18 10:21:24 +00:00 Compare
konrad force-pushed feature/projects-all-the-way-down from 9241c7e587 to ed8de7e3eb 2023-05-24 13:54:48 +00:00 Compare
konrad added 2 commits 2023-05-24 16:32:32 +00:00
konrad added 1 commit 2023-05-30 08:42:45 +00:00
continuous-integration/drone/pr Build was killed Details
7b05ed9d3d
fix: avoid crashing browser processes during tests
konrad added 4 commits 2023-05-30 09:09:20 +00:00
konrad added 1 commit 2023-05-30 09:41:51 +00:00
continuous-integration/drone/pr Build is passing Details
391992effb
fix: missing await
Author
Owner

From my point of view all comments are addressed and the tests are green. Merging now!

From my point of view all comments are addressed and the tests are green. Merging now!
konrad merged commit ac1d374191 into main 2023-05-30 10:09:40 +00:00
konrad deleted branch feature/projects-all-the-way-down 2023-05-30 10:09:40 +00:00
This repo is archived. You cannot comment on pull requests.
No description provided.