feature/projects-all-the-way-down #3323
156
src/components/home/ProjectsNavigation.vue
Normal file
|
|
@ -0,0 +1,156 @@
|
|||
<template>
|
||||
|
dpschen marked this conversation as resolved
Outdated
|
||||
<!-- v-if="projectsVisible[n.id] ?? true"-->
|
||||
|
dpschen marked this conversation as resolved
Outdated
dpschen
commented
For a new component: can we use the sortable from vueuse? For a new component: can we use the sortable from vueuse?
konrad
commented
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?
konrad
commented
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.
|
||||
<!-- :disabled="n.id < 0 || undefined"-->
|
||||
<!-- :modelValue="p"-->
|
||||
<!-- @update:modelValue="(projects) => updateActiveProjects(n, projects)"-->
|
||||
<!-- v-for="(p, pk) in projects"-->
|
||||
<!-- :key="p.id"-->
|
||||
<!-- :data-project-id="p.id"-->
|
||||
<!-- :data-project-index="pk"-->
|
||||
|
dpschen marked this conversation as resolved
Outdated
dpschen
commented
Use Use [`<menu>`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/menu) (now I remembered the correct element).
konrad
commented
Done Done
|
||||
<draggable
|
||||
v-model="availableProjects"
|
||||
v-bind="dragOptions"
|
||||
group="namespace-lists"
|
||||
@start="() => drag = true"
|
||||
@end="saveProjectPosition"
|
||||
handle=".handle"
|
||||
tag="ul"
|
||||
item-key="id"
|
||||
:component-data="{
|
||||
type: 'transition-group',
|
||||
name: !drag ? 'flip-list' : null,
|
||||
|
dpschen marked this conversation as resolved
Outdated
dpschen
commented
Use long var name. I thought for a bit that for 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`.
konrad
commented
Done. Done.
|
||||
class: [
|
||||
'menu-list can-be-hidden',
|
||||
{ 'dragging-disabled': false }
|
||||
]
|
||||
}"
|
||||
>
|
||||
|
dpschen
commented
Since this block doesn't have a headline it shouldn't be a Since this block doesn't have a headline it shouldn't be a `<section>`. Maybe use `<nav>` instead (nesting is allowed!)
dpschen
commented
Move this whole block in a new 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.
konrad
commented
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
konrad
commented
Shouldn't a Shouldn't a `nav` hold multiple navigation items?
dpschen
commented
Yes! Sry I misread the position, where the
Okay you moved now only the item without the list below inside.
This whole block can then be simplified: Because we can save the collapsed state inside each item we don't need to manage a list anymore. > 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)
})
```
dpschen
commented
So we don't even need the It's totally fine to not group the buttons etc because they are already grouped by the 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`
konrad
commented
That makes sense. I've moved most of the logic over, as you suggested.
We actually need this (or another element) because the 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.
|
||||
<template #item="{element: p}">
|
||||
<li
|
||||
class="list-menu loader-container is-loading-small"
|
||||
:class="{'is-loading': projectUpdating[p.id]}"
|
||||
>
|
||||
<BaseButton
|
||||
:to="{ name: 'project.index', params: { projectId: p.id} }"
|
||||
class="list-menu-link"
|
||||
:class="{'router-link-exact-active': currentProject.id === p.id}"
|
||||
>
|
||||
<span class="icon menu-item-icon handle">
|
||||
<icon icon="grip-lines"/>
|
||||
</span>
|
||||
<ColorBubble
|
||||
v-if="p.hexColor !== ''"
|
||||
:color="p.hexColor"
|
||||
class="mr-1"
|
||||
/>
|
||||
<span class="list-menu-title">{{ getProjectTitle(p) }}</span>
|
||||
</BaseButton>
|
||||
<BaseButton
|
||||
|
konrad marked this conversation as resolved
Outdated
dpschen
commented
Add types for emit Add types for emit
konrad
commented
Done Done
|
||||
v-if="p.id > 0"
|
||||
class="favorite"
|
||||
:class="{'is-favorite': p.isFavorite}"
|
||||
|
konrad marked this conversation as resolved
Outdated
dpschen
commented
These options should either contain all dragOptions or be defined inline These options should either contain all dragOptions or be defined inline
konrad
commented
Moved it all inline. Moved it all inline.
|
||||
@click="projectStore.toggleProjectFavorite(l)"
|
||||
>
|
||||
<icon :icon="p.isFavorite ? 'star' : ['far', 'star']"/>
|
||||
</BaseButton>
|
||||
<ProjectSettingsDropdown class="menu-list-dropdown" :project="p" v-if="p.id > 0">
|
||||
<template #trigger="{toggleOpen}">
|
||||
<BaseButton class="menu-list-dropdown-trigger" @click="toggleOpen">
|
||||
|
dpschen
commented
Is this even necessary if we use Is this even necessary if we use `modelValue` instead of `v-model` for the draggable?
konrad
commented
`v-model` is required, using `modelValue` for the draggable component does not work.
|
||||
<icon icon="ellipsis-h" class="icon"/>
|
||||
|
dpschen marked this conversation as resolved
Outdated
dpschen
commented
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?
konrad
commented
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.
dpschen
commented
My bad I confused favorite projects with the 'Favorite' project. My bad I confused favorite projects with the 'Favorite' project.
|
||||
</BaseButton>
|
||||
</template>
|
||||
</ProjectSettingsDropdown>
|
||||
<span class="list-setting-spacer" v-else></span>
|
||||
</li>
|
||||
</template>
|
||||
</draggable>
|
||||
|
||||
<!-- <ProjectsNavigation-->
|
||||
|
dpschen marked this conversation as resolved
Outdated
|
||||
<!-- v-if="projects.childProjects.length > 0"-->
|
||||
|
dpschen
commented
Also don't render if there are no child projects. Also don't render if there are no child projects.
konrad
commented
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.
|
||||
<!-- :projects="projects.childProjects"-->
|
||||
<!-- />-->
|
||||
</template>
|
||||
|
||||
<script lang="ts" setup>
|
||||
import {ref, computed, watch, onActivated} from 'vue'
|
||||
import draggable from 'zhyswan-vuedraggable'
|
||||
import type {SortableEvent} from 'sortablejs'
|
||||
|
||||
import BaseButton from '@/components/base/BaseButton.vue'
|
||||
import ProjectSettingsDropdown from '@/components/project/project-settings-dropdown.vue'
|
||||
|
||||
import {calculateItemPosition} from '@/helpers/calculateItemPosition'
|
||||
import {getProjectTitle} from '@/helpers/getProjectTitle'
|
||||
import type {IProject} from '@/modelTypes/IProject'
|
||||
import ColorBubble from '@/components/misc/colorBubble.vue'
|
||||
|
||||
import {useBaseStore} from '@/stores/base'
|
||||
import {useProjectStore} from '@/stores/projects'
|
||||
|
||||
const props = defineProps<{
|
||||
projects: IProject[],
|
||||
}>()
|
||||
const drag = ref(false)
|
||||
const dragOptions = {
|
||||
animation: 100,
|
||||
ghostClass: 'ghost',
|
||||
}
|
||||
|
dpschen marked this conversation as resolved
Outdated
dpschen
commented
Should have a default value. When is Should have a default value.
When is `allowDrag === true`? In case this is related to edit rights we should align the variable names.
dpschen
commented
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 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`?
konrad
commented
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
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 baseStore = useBaseStore()
|
||||
const projectStore = useProjectStore()
|
||||
const currentProject = computed(() => baseStore.currentProject)
|
||||
|
||||
// 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
|
||||
const availableProjects = ref<IProject[]>([])
|
||||
watch(
|
||||
|
dpschen
commented
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.
konrad
commented
It looks like it works fine without the emit. We're always binding with It looks like it works fine without the emit. We're always binding with `:modelValue` instead of `v-model` anyway…
|
||||
props.projects,
|
||||
projects => {
|
||||
availableProjects.value = projects
|
||||
},
|
||||
{immediate: true},
|
||||
|
dpschen marked this conversation as resolved
Outdated
dpschen
commented
What consequence has this? What consequence has this?
konrad
commented
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
Outdated
dpschen
commented
What exactly needs to be done for child projects? What exactly needs to be done for child projects?
konrad
commented
That was an old comment, I've removed it That was an old comment, I've removed it
|
||||
onActivated(() => availableProjects.value = props.projects)
|
||||
|
||||
const projectUpdating = ref<{ [id: IProject['id']]: boolean }>({})
|
||||
|
||||
async function saveProjectPosition(e: SortableEvent) {
|
||||
if (!e.newIndex && e.newIndex !== 0) return
|
||||
|
||||
const projectsActive = availableProjects.value
|
||||
// If the project was dragged to the last position, Safari will report e.newIndex as the size of the projectsActive
|
||||
// array instead of using the position. Because the index is wrong in that case, dragging the project will fail.
|
||||
// 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 projectBefore = projectsActive[newIndex - 1] ?? null
|
||||
const projectAfter = projectsActive[newIndex + 1] ?? null
|
||||
projectUpdating.value[project.id] = true
|
||||
|
||||
const position = calculateItemPosition(
|
||||
projectBefore !== null ? projectBefore.position : null,
|
||||
projectAfter !== null ? projectAfter.position : null,
|
||||
)
|
||||
|
||||
console.log({
|
||||
position,
|
||||
newIndex,
|
||||
project: project.id,
|
||||
projectBefore: projectBefore?.id,
|
||||
projectAfter: projectAfter?.id,
|
||||
})
|
||||
|
||||
try {
|
||||
// create a copy of the project in order to not violate pinia manipulation
|
||||
await projectStore.updateProject({
|
||||
...project,
|
||||
position,
|
||||
})
|
||||
} finally {
|
||||
projectUpdating.value[project.id] = false
|
||||
}
|
||||
}
|
||||
</script>
|
||||
|
|
@ -49,9 +49,7 @@
|
|||
</nav>
|
||||
|
||||
<nav>
|
||||
|
dpschen marked this conversation as resolved
Outdated
dpschen
commented
Move Move `v-if` to front
konrad
commented
Done Done
|
||||
<template v-for="(p, pk) in projects" :key="p.id">
|
||||
{{ p. title }}<br/>
|
||||
</template>
|
||||
<ProjectsNavigation :projects="projects"/>
|
||||
</nav>
|
||||
|
||||
<!-- <nav class="menu namespaces-lists loader-container is-loading-small" :class="{'is-loading': loading}">-->
|
||||
|
|
@ -84,68 +82,7 @@
|
|||
<!-- NOTE: a v-model / computed setter is not possible, since the updateActiveProjects function-->
|
||||
|
dpschen marked this conversation as resolved
Outdated
dpschen
commented
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.
konrad
commented
That makes sense. That makes sense.
konrad
commented
Changed it. Changed it.
|
||||
<!-- triggered by the change needs to have access to the current namespace-->
|
||||
<!-- –>-->
|
||||
|
dpschen marked this conversation as resolved
Outdated
dpschen
commented
Simplify to Simplify to
```ts
.sort((a, b) => a.position - b.position)
```
konrad
commented
Done. Done.
|
||||
<!-- <draggable-->
|
||||
<!-- v-if="projectsVisible[n.id] ?? true"-->
|
||||
<!-- v-bind="dragOptions"-->
|
||||
<!-- :modelValue="activeProjects[nk]"-->
|
||||
<!-- @update:modelValue="(projects) => updateActiveProjects(n, projects)"-->
|
||||
<!-- group="namespace-lists"-->
|
||||
<!-- @start="() => drag = true"-->
|
||||
<!-- @end="saveListPosition"-->
|
||||
<!-- handle=".handle"-->
|
||||
<!-- :disabled="n.id < 0 || undefined"-->
|
||||
<!-- tag="ul"-->
|
||||
<!-- item-key="id"-->
|
||||
<!-- :data-namespace-id="n.id"-->
|
||||
<!-- :data-namespace-index="nk"-->
|
||||
<!-- :component-data="{-->
|
||||
<!-- type: 'transition-group',-->
|
||||
<!-- name: !drag ? 'flip-list' : null,-->
|
||||
<!-- class: [-->
|
||||
<!-- 'menu-list can-be-hidden',-->
|
||||
<!-- { 'dragging-disabled': n.id < 0 }-->
|
||||
<!-- ]-->
|
||||
<!-- }"-->
|
||||
<!-- >-->
|
||||
<!-- <template #item="{element: l}">-->
|
||||
<!-- <li-->
|
||||
<!-- class="list-menu loader-container is-loading-small"-->
|
||||
<!-- :class="{'is-loading': projectUpdating[l.id]}"-->
|
||||
<!-- >-->
|
||||
<!-- <BaseButton-->
|
||||
<!-- :to="{ name: 'project.index', params: { projectId: l.id} }"-->
|
||||
<!-- class="list-menu-link"-->
|
||||
<!-- :class="{'router-link-exact-active': currentProject.id === l.id}"-->
|
||||
<!-- >-->
|
||||
<!-- <span class="icon menu-item-icon handle">-->
|
||||
<!-- <icon icon="grip-lines"/>-->
|
||||
<!-- </span>-->
|
||||
<!-- <ColorBubble-->
|
||||
<!-- v-if="l.hexColor !== ''"-->
|
||||
<!-- :color="l.hexColor"-->
|
||||
<!-- class="mr-1"-->
|
||||
<!-- />-->
|
||||
<!-- <span class="list-menu-title">{{ getProjectTitle(l) }}</span>-->
|
||||
<!-- </BaseButton>-->
|
||||
<!-- <BaseButton-->
|
||||
<!-- v-if="l.id > 0"-->
|
||||
<!-- class="favorite"-->
|
||||
<!-- :class="{'is-favorite': l.isFavorite}"-->
|
||||
<!-- @click="projectStore.toggleProjectFavorite(l)"-->
|
||||
<!-- >-->
|
||||
<!-- <icon :icon="l.isFavorite ? 'star' : ['far', 'star']"/>-->
|
||||
<!-- </BaseButton>-->
|
||||
<!-- <ProjectSettingsDropdown class="menu-list-dropdown" :project="l" v-if="l.id > 0">-->
|
||||
<!-- <template #trigger="{toggleOpen}">-->
|
||||
<!-- <BaseButton class="menu-list-dropdown-trigger" @click="toggleOpen">-->
|
||||
<!-- <icon icon="ellipsis-h" class="icon"/>-->
|
||||
<!-- </BaseButton>-->
|
||||
<!-- </template>-->
|
||||
<!-- </ProjectSettingsDropdown>-->
|
||||
<!-- <span class="list-setting-spacer" v-else></span>-->
|
||||
<!-- </li>-->
|
||||
<!-- </template>-->
|
||||
<!-- </draggable>-->
|
||||
|
||||
<!-- </template>-->
|
||||
<!-- </nav>-->
|
||||
<PoweredByLink/>
|
||||
|
dpschen
commented
This sorts the returned computed of the store Error in vue developer tools:
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. [...]
konrad
commented
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)
|
||||
|
|
@ -170,15 +107,10 @@ import ColorBubble from '@/components/misc/colorBubble.vue'
|
|||
|
||||
import {useBaseStore} from '@/stores/base'
|
||||
import {useProjectStore} from '@/stores/projects'
|
||||
import ProjectsNavigation from '@/components/home/ProjectsNavigation.vue'
|
||||
|
||||
const drag = ref(false)
|
||||
const dragOptions = {
|
||||
animation: 100,
|
||||
ghostClass: 'ghost',
|
||||
}
|
||||
|
||||
const baseStore = useBaseStore()
|
||||
const currentProject = computed(() => baseStore.currentProject)
|
||||
const menuActive = computed(() => baseStore.menuActive)
|
||||
const loading = computed(() => namespaceStore.isLoading)
|
||||
|
||||
|
dpschen
commented
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.
|
||||
|
|
@ -214,7 +146,7 @@ onBeforeMount(async () => {
|
|||
await projectStore.loadProjects()
|
||||
})
|
||||
|
||||
const projects = computed(() => projectStore.projects)
|
||||
const projects = computed(() => Object.values(projectStore.projects).sort((a, b) => a.position < b.position ? -1 : 1))
|
||||
|
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
|
||||
|
||||
function updateActiveProjects(namespace: INamespace, activeProjects: IProject[]) {
|
||||
// This is a bit hacky: since we do have to filter out the archived items from the list
|
||||
|
|
@ -233,41 +165,7 @@ function updateActiveProjects(namespace: INamespace, activeProjects: IProject[])
|
|||
})
|
||||
}
|
||||
|
||||
const projectUpdating = ref<{ [id: INamespace['id']]: boolean }>({})
|
||||
|
||||
async function saveProjectPosition(e: SortableEvent) {
|
||||
if (!e.newIndex && e.newIndex !== 0) return
|
||||
|
||||
const namespaceId = parseInt(e.to.dataset.namespaceId as string)
|
||||
const newNamespaceIndex = parseInt(e.to.dataset.namespaceIndex as string)
|
||||
|
||||
const projectsActive = activeProjects.value[newNamespaceIndex]
|
||||
// If the project was dragged to the last position, Safari will report e.newIndex as the size of the projectsActive
|
||||
// array instead of using the position. Because the index is wrong in that case, dragging the project will fail.
|
||||
// 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 projectBefore = projectsActive[newIndex - 1] ?? null
|
||||
const projectAfter = projectsActive[newIndex + 1] ?? null
|
||||
projectUpdating.value[project.id] = true
|
||||
|
||||
const position = calculateItemPosition(
|
||||
projectBefore !== null ? projectBefore.position : null,
|
||||
projectAfter !== null ? projectAfter.position : null,
|
||||
)
|
||||
|
||||
try {
|
||||
// create a copy of the project in order to not violate pinia manipulation
|
||||
await projectStore.updateProject({
|
||||
...project,
|
||||
position,
|
||||
namespaceId,
|
||||
})
|
||||
} finally {
|
||||
projectUpdating.value[project.id] = false
|
||||
}
|
||||
}
|
||||
</script>
|
||||
|
||||
<style lang="scss" scoped>
|
||||
|
|
|
|||
|
|
@ -18,6 +18,7 @@ export interface IProject extends IAbstract {
|
|||
subscription: ISubscription
|
||||
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).
|
||||
|
||||
created: Date
|
||||
updated: Date
|
||||
|
|
|
|||
|
|
@ -115,11 +115,9 @@ export const useProjectStore = defineStore('project', () => {
|
|||
|
||||
// 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
|
||||
|
konrad marked this conversation as resolved
Outdated
dpschen
commented
- 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?
konrad
commented
Done. Done.
|
||||
const newProject = {
|
||||
return {
|
||||
...project,
|
||||
}
|
||||
|
||||
return newProject
|
||||
} catch (e) {
|
||||
// Reset the project state to the initial one to avoid confusion for the user
|
||||
setProject({
|
||||
|
|
|
|||
This component is basically a redo of a huge part of what I did with https://kolaente.dev/vikunja/frontend/pulls/2108/files
Looks like it. Do you want to continue that PR? Given how it is already old and now even more outdated.
I worked many hours to untangle the CSS there. I hope that we can save that effort somehow.
From a quick glance over it, it seems like a big part of that was the namespace title styles - which are now gone (since namespaces are gone).
Probably the naming was bad. If I remember correctly the NavigationNamespace component was compatible with projects and namespaces. Not recursive though. Will check at some point how to recover good stuff.