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
5 changed files with 36 additions and 20 deletions
Showing only changes of commit 760efa854d - Show all commits

View File

@ -26,7 +26,7 @@
>
<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.

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

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`

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

Nice!

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

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.
v-model="p.childProjects"
v-model="childProjects[p.id]"
:can-edit-order="true"
/>
</li>
@ -93,7 +93,7 @@ import {useBaseStore} from '@/stores/base'
import {useProjectStore} from '@/stores/projects'
const props = defineProps<{
modelValue: IProject[],
modelValue?: IProject[],
canEditOrder: boolean,
dpschen marked this conversation as resolved Outdated

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

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

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

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.
const emit = defineEmits(['update:modelValue'])
@ -114,11 +114,16 @@ const currentProject = computed(() => baseStore.currentProject)
// TODO: child projects
dpschen marked this conversation as resolved Outdated

What exactly needs to be done for child projects?

What exactly needs to be done for child projects?

That was an old comment, I've removed it

That was an old comment, I've removed it
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)
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},
)
@ -149,8 +154,8 @@ async function saveProjectPosition(e: SortableEvent) {
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 {

View File

@ -17,14 +17,18 @@ 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.

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

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?

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.

No worries, changed it back.

No worries, changed it back.
const projects = computed(() => projectStore.projectsArray
.filter(p => p.parentProjectId === 0 && !p.isArchived)
.sort((a, b) => a.position - b.position))
const projects = computed({
konrad marked this conversation as resolved Outdated

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.

Done

Done
get() {
return projectStore.projectsArray
.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.
konrad marked this conversation as resolved Outdated

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"`

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

I've now changed it to `:modelValue` and that seems to work.
})
const favoriteProjects = computed(() => projectStore.projectsArray
konrad marked this conversation as resolved Outdated

favoriteProjects also seems like something a store should export.

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

Done

Done

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); } ```

Done.

Done.
.filter(p => !p.isArchived && p.isFavorite)
.map(p => ({
konrad marked this conversation as resolved Outdated

Why the mapping / returning of a copy?

Why the mapping / returning of a copy?

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

That was a relic, I've removed it now.
...p,
childProjects: [],
}))
.sort((a, b) => a.position - b.position))
</script>

View File

@ -18,7 +18,7 @@ export interface IProject extends IAbstract {
subscription: ISubscription
position: number
backgroundBlurHash: string
childProjects: IProject[] | null
childProjectIds: number[]

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.

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…

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.

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.

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.

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.

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?

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)

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).
parentProjectId: number
created: Date

View File

@ -22,7 +22,7 @@ export default class ProjectModel extends AbstractModel<IProject> implements IPr
subscription: ISubscription = null
position = 0
backgroundBlurHash = ''
childProjects = []
childProjectIds = []
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`?

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.
created: Date = null
@ -47,7 +47,7 @@ export default class ProjectModel extends AbstractModel<IProject> implements IPr
this.subscription = new SubscriptionModel(this.subscription)
}
this.childProjects = this.childProjects.map(p => new ProjectModel(p))
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?

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.
this.created = new Date(this.created)
this.updated = new Date(this.updated)

View File

@ -34,6 +34,9 @@ export const useProjectStore = defineStore('project', () => {
const getProjectById = computed(() => {
return (id: IProject['id']) => typeof projects.value[id] !== 'undefined' ? projects.value[id] : null
})
const getChildProjects = computed(() => {

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

Fixed.

Fixed.
return (id: IProject['id']) => projectsArray.value.filter(p => p.parentProjectId === id) || []
})

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]) ```

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.
const findProjectByExactname = computed(() => {
return (name: string) => {
@ -67,24 +70,27 @@ export const useProjectStore = defineStore('project', () => {
}
if (updateChildren) {
project.childProjects?.forEach(p => setProject(p))
// When projects are loaded from the api, they will include child projects

See comment about having the data as sub projects from earlier.

See comment about having the data as sub projects from earlier.
// in the `childProjects` property. We flatten them out into the project store here.
project.childProjects?.forEach(p => setProject(new ProjectModel(p)))
delete project.childProjects

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.

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)

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.

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.
}
// 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 +203,7 @@ export const useProjectStore = defineStore('project', () => {
hasProjects: readonly(hasProjects),
getProjectById,
getChildProjects,
findProjectByExactname,
searchProject,