feature/projects-all-the-way-down #3323
36
src/components/home/ProjectsNavigationWrapper.vue
Normal file
|
|
@ -0,0 +1,36 @@
|
|||
<template>
|
||||
<nav class="menu" v-if="favoriteProjects">
|
||||
|
|
||||
<ProjectsNavigation v-model="favoriteProjects" :can-edit-order="false"/>
|
||||
</nav>
|
||||
|
||||
<nav class="menu">
|
||||
<ProjectsNavigation v-model="projects" :can-edit-order="true"/>
|
||||
</nav>
|
||||
</template>
|
||||
|
||||
<script setup lang="ts">
|
||||
import {computed} from 'vue'
|
||||
import {useProjectStore} from '@/stores/projects'
|
||||
import ProjectsNavigation from '@/components/home/ProjectsNavigation.vue'
|
||||
|
||||
const projectStore = useProjectStore()
|
||||
|
||||
await projectStore.loadProjects()
|
||||
dpschen
commented
Call this in 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.
konrad
commented
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.
dpschen
commented
The loading of the projects would happen much earlier in the request chain. 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).
konrad
commented
I've now moved it to 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.
dpschen
commented
That makes sense. 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?
konrad
commented
Yes, I created it only to use it with Yes, I created it only to use it with `Suspense`. Should I include its contents back into `navigation`?
dpschen
commented
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.
konrad
commented
No worries, changed it back. No worries, changed it back.
|
||||
|
||||
const projects = computed(() => projectStore.projectsArray
|
||||
|
konrad marked this conversation as resolved
Outdated
dpschen
commented
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.
konrad
commented
Done Done
|
||||
.filter(p => p.parentProjectId === 0 && !p.isArchived)
|
||||
.sort((a, b) => a.position - b.position))
|
||||
const favoriteProjects = computed(() => projectStore.projectsArray
|
||||
.filter(p => !p.isArchived && p.isFavorite)
|
||||
.map(p => ({
|
||||
...p,
|
||||
|
konrad marked this conversation as resolved
Outdated
dpschen
commented
Probably because you use 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"`
konrad
commented
I've now changed it to I've now changed it to `:modelValue` and that seems to work.
|
||||
childProjects: [],
|
||||
}))
|
||||
|
konrad marked this conversation as resolved
Outdated
dpschen
commented
`favoriteProjects` also seems like something a store should export.
konrad
commented
Done Done
dpschen
commented
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 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);
}
```
konrad
commented
Done. Done.
|
||||
.sort((a, b) => a.position - b.position))
|
||||
</script>
|
||||
|
konrad marked this conversation as resolved
Outdated
dpschen
commented
Why the mapping / returning of a copy? Why the mapping / returning of a copy?
konrad
commented
That was a relic, I've removed it now. That was a relic, I've removed it now.
|
||||
|
||||
<style lang="scss" scoped>
|
||||
.menu {
|
||||
padding-top: math.div($navbar-padding, 2);
|
||||
}
|
||||
</style>
|
||||
|
|
@ -16,7 +16,12 @@
|
|||
:class="{'is-visible': background}"
|
||||
class="app-container-background background-fade-in d-print-none"
|
||||
:style="{'background-image': background && `url(${background})`}"></div>
|
||||
<navigation class="d-print-none"/>
|
||||
<Suspense>
|
||||
<navigation class="d-print-none"/>
|
||||
<template #fallback>
|
||||
Loading...
|
||||
|
dpschen marked this conversation as resolved
Outdated
dpschen
commented
Use Use `components/misc/loading.vue`
konrad
commented
That was an old test I did, I've now removed it. That was an old test I did, I've now removed it.
|
||||
</template>
|
||||
</Suspense>
|
||||
<main
|
||||
class="app-content"
|
||||
:class="[
|
||||
|
|
|
|||
|
|
@ -48,47 +48,30 @@
|
|||
</ul>
|
||||
</nav>
|
||||
|
||||
<nav class="menu" v-if="favoriteProjects">
|
||||
<ProjectsNavigation v-model="favoriteProjects" :can-edit-order="false"/>
|
||||
</nav>
|
||||
<Suspense>
|
||||
|
dpschen marked this conversation as resolved
Outdated
dpschen
commented
Move Move `v-if` to front
konrad
commented
Done Done
|
||||
<ProjectsNavigationWrapper/>
|
||||
|
||||
<nav class="menu">
|
||||
<ProjectsNavigation v-model="projects" :can-edit-order="true"/>
|
||||
</nav>
|
||||
<template #fallback>
|
||||
<Loading/>
|
||||
</template>
|
||||
</Suspense>
|
||||
|
||||
<PoweredByLink/>
|
||||
</aside>
|
||||
</template>
|
||||
|
||||
<script setup lang="ts">
|
||||
import {computed, onBeforeMount} from 'vue'
|
||||
import {computed} from 'vue'
|
||||
|
||||
import PoweredByLink from '@/components/home/PoweredByLink.vue'
|
||||
import Logo from '@/components/home/Logo.vue'
|
||||
import Loading from '@/components/misc/loading.vue'
|
||||
|
||||
import {useBaseStore} from '@/stores/base'
|
||||
import {useProjectStore} from '@/stores/projects'
|
||||
import ProjectsNavigation from '@/components/home/ProjectsNavigation.vue'
|
||||
import ProjectsNavigationWrapper from '@/components/home/ProjectsNavigationWrapper.vue'
|
||||
|
||||
const baseStore = useBaseStore()
|
||||
const projectStore = useProjectStore()
|
||||
const menuActive = computed(() => baseStore.menuActive)
|
||||
|
||||
// FIXME: async action will be unfinished when component mounts
|
||||
onBeforeMount(async () => {
|
||||
await projectStore.loadProjects()
|
||||
})
|
||||
|
||||
const projects = computed(() => projectStore.projectsArray
|
||||
.filter(p => p.parentProjectId === 0 && !p.isArchived)
|
||||
.sort((a, b) => a.position - b.position))
|
||||
const favoriteProjects = computed(() => projectStore.projectsArray
|
||||
.filter(p => !p.isArchived && p.isFavorite)
|
||||
.map(p => ({
|
||||
...p,
|
||||
childProjects: [],
|
||||
}))
|
||||
.sort((a, b) => a.position - b.position))
|
||||
</script>
|
||||
|
||||
<style lang="scss" scoped>
|
||||
|
dpschen
commented
Can we put this component inside a Can we put this component inside a `<Suspense>`? Then we can use `await` methods directly and without `onBeforeMount ` hook.
konrad
commented
I've now moved the project navigation into a separate wrapper component so that we can show a loading spinner while projects are loading and still show the other navigation links (overview, labels, etc). I've now moved the project navigation into a separate wrapper component so that we can show a loading spinner while projects are loading and still show the other navigation links (overview, labels, etc).
dpschen
commented
Ok. Will check Ok. Will check
|
||||
|
|
@ -150,7 +133,16 @@ const favoriteProjects = computed(() => projectStore.projectsArray
|
|||
}
|
||||
}
|
||||
|
||||
.menu {
|
||||
padding-top: math.div($navbar-padding, 2);
|
||||
.loader-container {
|
||||
|
konrad marked this conversation as resolved
Outdated
dpschen
commented
This class doesn't isn't used in the template This class doesn't isn't used in the template
konrad
commented
I've renamed it - it is coming from the I've renamed it - it is coming from the `Loader` component.
|
||||
min-width: 100%;
|
||||
height: 150px;
|
||||
|
||||
&.is-loading::after {
|
||||
|
dpschen marked this conversation as resolved
Outdated
dpschen
commented
This changes styles inside the component. If the styles need to be adjusted the component should be changed instead. This changes styles inside the component. If the styles need to be adjusted the component should be changed instead.
konrad
commented
The problem is the component is used in multiple places where this would need different sizes. The way to do this would probably be variants with a prop? The problem is the component is used in multiple places where this would need different sizes. The way to do this would probably be variants with a prop?
dpschen
commented
Correct! The question to ask is also: do we even need so many different sizes or shouldn't they be more unied. For now I guess it's enough to answer this quesion for this usecase here. Correct! The question to ask is also: do we even need so many different sizes or shouldn't they be more unied. For now I guess it's enough to answer this quesion for this usecase here.
konrad
commented
I've now added this as a variant to the component. In doing this I discovered there are more styles and uses of that loader which we should refactor at some point. I would consider that out of scope for this PR though. I've now added this as a variant to the component. In doing this I discovered there are more styles and uses of that loader which we should refactor at some point. I would consider that out of scope for this PR though.
|
||||
width: 3rem;
|
||||
height: 3rem;
|
||||
top: calc(50% - 1.5rem);
|
||||
|
dpschen marked this conversation as resolved
Outdated
konrad
commented
Done Done
|
||||
left: calc(50% - 1.5rem);
|
||||
border-width: 3px;
|
||||
}
|
||||
}
|
||||
</style>
|
||||
|
|
|
|||
Didn't check / thought: I think to remember that
menuis a class also coming from bulma.If that's the case: does that mean that we pass these styles down here? Could we instead add the menu class inside ProjectsNavigation or wouldn't that work because of the recurrsion?
Yes,
.menuis a class from bulma.You mean adding the class into the styles definition of the
ProjectsNavigationcomponent?