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
7 changed files with 29 additions and 3 deletions
Showing only changes of commit cb218ec0c3 - Show all commits

View File

@ -54,6 +54,7 @@ ENV VIKUNJA_LOG_FORMAT main
ENV VIKUNJA_API_URL /api/v1
ENV VIKUNJA_SENTRY_ENABLED false
ENV VIKUNJA_SENTRY_DSN https://85694a2d757547cbbc90cd4b55c5a18d@o1047380.ingest.sentry.io/6024480
ENV VIKUNJA_INFINITE_PROJECT_NESTING_ENABLED false

Add types to env.d.ts

Add types to `env.d.ts`

Even though this env variable only works in Docker and is translated to a window. variable at runtime? We don't do this for the other variables either...

Even though this env variable only works in Docker and is translated to a window. variable at runtime? We don't do this for the other variables either...
COPY docker/injector.sh /docker-entrypoint.d/50-injector.sh
COPY docker/ipv6-disable.sh /docker-entrypoint.d/60-ipv6-disable.sh

View File

@ -11,5 +11,6 @@ VIKUNJA_SENTRY_DSN="$(echo "$VIKUNJA_SENTRY_DSN" | sed -r 's/([:;])/\\\1/g')"
sed -ri "s:^(\s*window.API_URL\s*=)\s*.+:\1 '${VIKUNJA_API_URL}':g" /usr/share/nginx/html/index.html
sed -ri "s:^(\s*window.SENTRY_ENABLED\s*=)\s*.+:\1 ${VIKUNJA_SENTRY_ENABLED}:g" /usr/share/nginx/html/index.html
sed -ri "s:^(\s*window.SENTRY_DSN\s*=)\s*.+:\1 '${VIKUNJA_SENTRY_DSN}':g" /usr/share/nginx/html/index.html
sed -ri "s:^(\s*window.INFINITE_PROJECT_NESTING_ENABLED\s*=)\s*.+:\1 '${VIKUNJA_INFINITE_PROJECT_NESTING_ENABLED}':g" /usr/share/nginx/html/index.html
date -uIseconds | xargs echo 'info: started at'

View File

@ -27,6 +27,9 @@
// our sentry instance to notify us of potential problems.
window.SENTRY_ENABLED = false
window.SENTRY_DSN = 'https://85694a2d757547cbbc90cd4b55c5a18d@o1047380.ingest.sentry.io/6024480'
// If enabled, allows the user to nest projects infinitely, instead of the default 2 levels.
// This setting might change in the future or be removed completely.
window.INFINITE_PROJECT_NESTING_ENABLED = false
</script>
</body>
</html>

View File

@ -23,6 +23,7 @@
:project="project"
:is-loading="projectUpdating[project.id]"
:can-collapse="canCollapse"
:level="level"
/>

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.
</template>
</draggable>
@ -44,6 +45,7 @@ const props = defineProps<{
modelValue?: IProject[],
canEditOrder: boolean,
canCollapse?: boolean,
level?: number,
konrad marked this conversation as resolved Outdated

Add types for emit

Add types for emit

Done

Done
}>()
const emit = defineEmits(['update:modelValue'])
konrad marked this conversation as resolved Outdated

These options should either contain all dragOptions or be defined inline

These options should either contain all dragOptions or be defined inline

Moved it all inline.

Moved it all inline.

View File

@ -46,10 +46,11 @@
<span class="list-setting-spacer" v-else></span>
</section>
<ProjectsNavigation
v-if="childProjectsOpen && canCollapse"
v-if="canNestDeeper && childProjectsOpen && canCollapse"

Use Expandable component for this.

Use `Expandable` component for this.

This seems to completly break the styling. I changed it to match the selectors but it still does not work. Not sure what to do about this.-

This seems to completly break the styling. I changed it to match the selectors but it still does not work. Not sure what to do about this.-

I created an example how to use this in 51e29af010. I was unsure which parts parts should be dynamically be filled (via the open prop) or static (not rendering the Expandable at all via v-if).

I created an example how to use this in https://kolaente.dev/vikunja/frontend/commit/51e29af010defc5f6c46f85dbb7311904a7d40e1. I was unsure which parts parts should be dynamically be filled (via the `open` prop) or static (not rendering the `Expandable` at all via `v-if`).
v-model="childProjects"
:can-edit-order="true"
:can-collapse="canCollapse"
:level="level + 1"
/>
</li>
</template>
@ -71,6 +72,7 @@ const props = defineProps<{
project: IProject,
isLoading?: boolean,
canCollapse?: boolean,
level?: number,
}>()
const projectStore = useProjectStore()
@ -80,10 +82,21 @@ const currentProject = computed(() => baseStore.currentProject)
const childProjectsOpen = ref(true)
const childProjects = computed(() => {
if (!canNestDeeper.value) {
return []
}
return projectStore.getChildProjects(props.project.id)
.sort((a, b) => a.position - b.position)
})
const canNestDeeper = computed(() => {
if (props.level < 2) {

Simplify:

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

But that would return false for the first two levels?

But that would return `false` for the first two levels?
return true
}
return props.level >= 2 && window.INFINITE_PROJECT_NESTING_ENABLED
})
</script>
<style lang="scss" scoped>

View File

@ -55,7 +55,12 @@
</nav>
<nav class="menu">
<ProjectsNavigation :model-value="projects" :can-edit-order="true" :can-collapse="true"/>
<ProjectsNavigation
:model-value="projects"
:can-edit-order="true"
:can-collapse="true"
:level="1"
/>
</nav>
</template>
@ -140,7 +145,7 @@ const favoriteProjects = computed(() => projectStore.favoriteProjects
}
}
.menu + .menu{
.menu + .menu {
padding-top: math.div($navbar-padding, 2);
konrad marked this conversation as resolved Outdated

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!

Re-added the space to the logo

Re-added the space to the logo
}
</style>

View File

@ -23,6 +23,7 @@ declare global {
API_URL: string;
SENTRY_ENABLED: boolean;
SENTRY_DSN: string;
INFINITE_PROJECT_NESTING_ENABLED: boolean;
konrad marked this conversation as resolved Outdated

PROJECT_INFINITE_NESTING_ENABLED

`PROJECT_INFINITE_NESTING_ENABLED`

Done.

Done.
}
}