feature/projects-all-the-way-down #3323
|
|
@ -5,7 +5,6 @@
|
|||
<!-- @update:modelValue="(projects) => updateActiveProjects(n, projects)"-->
|
||||
<!-- v-for="(p, pk) in projects"-->
|
||||
<!-- :key="p.id"-->
|
||||
<!-- :data-project-id="p.id"-->
|
||||
<!-- :data-project-index="pk"-->
|
||||
<draggable
|
||||
|
dpschen marked this conversation as resolved
Outdated
|
||||
v-model="availableProjects"
|
||||
|
|
@ -29,6 +28,7 @@
|
|||
<li
|
||||
class="list-menu loader-container is-loading-small"
|
||||
:class="{'is-loading': projectUpdating[p.id]}"
|
||||
:data-project-id="p.id"
|
||||
>
|
||||
<section>
|
||||
<BaseButton
|
||||
|
|
@ -73,7 +73,7 @@
|
|||
</section>
|
||||
<ProjectsNavigation
|
||||
v-if="p.childProjects.length > 0 && !collapsedProjects[p.id]"
|
||||
:projects="p.childProjects"
|
||||
v-model="p.childProjects"
|
||||
/>
|
||||
</li>
|
||||
</template>
|
||||
|
|
@ -98,8 +98,10 @@ import {useBaseStore} from '@/stores/base'
|
|||
import {useProjectStore} from '@/stores/projects'
|
||||
dpschen
commented
Danger! This should be handled in the store! Probably it would be best to create a new store method. Something like 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`.
konrad
commented
I was able to move the whole thing into the I was able to move the whole thing into the `updateProject` method of the store.
|
||||
|
||||
const props = defineProps<{
|
||||
projects: IProject[],
|
||||
modelValue: IProject[],
|
||||
}>()
|
||||
const emit = defineEmits(['update:modelValue'])
|
||||
|
||||
const drag = ref(false)
|
||||
const dragOptions = {
|
||||
animation: 100,
|
||||
|
|
@ -117,7 +119,7 @@ const currentProject = computed(() => baseStore.currentProject)
|
|||
const collapsedProjects = ref<{ [id: IProject['id']]: boolean }>({})
|
||||
const availableProjects = ref<IProject[]>([])
|
||||
watch(
|
||||
() => props.projects,
|
||||
() => props.modelValue,
|
||||
projects => {
|
||||
availableProjects.value = projects
|
||||
projects.forEach(p => collapsedProjects.value[p.id] = false)
|
||||
|
|
@ -136,7 +138,10 @@ async function saveProjectPosition(e: SortableEvent) {
|
|||
// To work around that we're explicitly checking that case here and decrease the index.
|
||||
const newIndex = e.newIndex === projectsActive.length ? e.newIndex - 1 : e.newIndex
|
||||
|
||||
const project = projectsActive[newIndex]
|
||||
const projectId = parseInt(e.item.dataset.projectId)
|
||||
const project = projectStore.getProjectById(projectId)
|
||||
|
||||
const parentProjectId = e.to.parentNode.dataset.projectId ? parseInt(e.to.parentNode.dataset.projectId) : 0
|
||||
const projectBefore = projectsActive[newIndex - 1] ?? null
|
||||
const projectAfter = projectsActive[newIndex + 1] ?? null
|
||||
projectUpdating.value[project.id] = true
|
||||
|
|
@ -159,7 +164,9 @@ async function saveProjectPosition(e: SortableEvent) {
|
|||
await projectStore.updateProject({
|
||||
...project,
|
||||
position,
|
||||
parentProjectId,
|
||||
})
|
||||
emit('update:modelValue', availableProjects.value)
|
||||
} finally {
|
||||
projectUpdating.value[project.id] = false
|
||||
}
|
||||
|
|
|
|||
|
|
@ -49,7 +49,7 @@
|
|||
</nav>
|
||||
|
||||
<nav class="menu namespaces-lists">
|
||||
|
dpschen marked this conversation as resolved
Outdated
dpschen
commented
Move Move `v-if` to front
konrad
commented
Done Done
|
||||
<ProjectsNavigation :projects="projects"/>
|
||||
<ProjectsNavigation v-model="projects"/>
|
||||
</nav>
|
||||
|
||||
<!-- <nav class="menu namespaces-lists loader-container is-loading-small" :class="{'is-loading': loading}">-->
|
||||
|
|
@ -146,7 +146,9 @@ onBeforeMount(async () => {
|
|||
await projectStore.loadProjects()
|
||||
})
|
||||
|
||||
const projects = computed(() => Object.values(projectStore.projects).sort((a, b) => a.position < b.position ? -1 : 1))
|
||||
const projects = computed(() => Object.values(projectStore.projects)
|
||||
|
konrad marked this conversation as resolved
Outdated
dpschen
commented
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!
konrad
commented
Re-added the space to the logo Re-added the space to the logo
|
||||
.filter(p => p.parentProjectId === 0)
|
||||
.sort((a, b) => a.position < b.position ? -1 : 1))
|
||||
|
||||
function updateActiveProjects(namespace: INamespace, activeProjects: IProject[]) {
|
||||
// This is a bit hacky: since we do have to filter out the archived items from the list
|
||||
|
|
|
|||
|
|
@ -19,6 +19,7 @@ export interface IProject extends IAbstract {
|
|||
position: number
|
||||
backgroundBlurHash: string
|
||||
childProjects: IProject[] | null
|
||||
|
dpschen
commented
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 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.
konrad
commented
What's that? The projects in > we should replace this with an array of `childProjects` instead.
What's that? The projects in `childProjects` are just regular projects.
dpschen
commented
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.
dpschen
commented
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…
konrad
commented
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 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.
dpschen
commented
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.
dpschen
commented
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.
konrad
commented
The api only uses the > 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.
konrad
commented
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.
dpschen
commented
Okay that means that we could simply return ids of the childProjects via a new property (e.g.)
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?
dpschen
commented
Note that the store still could provide a 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.
konrad
commented
I think we should be able to, yes. Would you add that as a new property to the Project Model and then map it out in the constructor?
Not really, since we need to fetch all children anyway. > 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.
konrad
commented
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?
konrad
commented
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 Here's what I did: 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)
konrad
commented
I got something working! See 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
|
||||
updated: Date
|
||||
|
|
|
|||
|
|
@ -23,6 +23,7 @@ export default class ProjectModel extends AbstractModel<IProject> implements IPr
|
|||
position = 0
|
||||
backgroundBlurHash = ''
|
||||
childProjects = []
|
||||
parentProjectId = 0
|
||||
|
dpschen
commented
Coming from the recent discussion: wouldn't it be better if this was Coming from the recent discussion: wouldn't it be better if this was `undefined` or `null`?
konrad
commented
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?
dpschen
commented
Okay, wasn't aware of this. So the EDIT: 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
|
||||
updated: Date = null
|
||||
|
|
|
|||
|
|
@ -62,6 +62,8 @@ export const useProjectStore = defineStore('project', () => {
|
|||
function setProject(project: IProject) {
|
||||
projects.value[project.id] = project
|
||||
update(project)
|
||||
|
||||
project.childProjects?.forEach(setProject)
|
||||
|
||||
if (baseStore.currentProject?.id === project.id) {
|
||||
baseStore.setCurrentProject(project)
|
||||
|
|
@ -69,10 +71,7 @@ export const useProjectStore = defineStore('project', () => {
|
|||
}
|
||||
|
||||
function setProjects(newProjects: IProject[]) {
|
||||
|
dpschen
commented
See comment about having the data as sub projects from earlier. See comment about having the data as sub projects from earlier.
|
||||
newProjects.forEach(l => {
|
||||
projects.value[l.id] = l
|
||||
add(l)
|
||||
})
|
||||
newProjects.forEach(setProject)
|
||||
}
|
||||
|
||||
|
dpschen
commented
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.
konrad
commented
You mean setting the current project in the base store? You mean setting the current project in the base store?
dpschen
commented
I mean:
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)
konrad
commented
I've now changed it to use a watcher.
That sounds like a good idea, but let's push that to another PR. I've now changed it to use a watcher.
> 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.
konrad
commented
I've now changed it to use a watcher.
That sounds like a good idea, but let's push that to another PR. I've now changed it to use a watcher.
> 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.
|
||||
function removeProjectById(project: IProject) {
|
||||
|
|
|
|||
Use
<menu>(now I remembered the correct element).Done