feat: replace our home-grown gantt implementation with ganttastic #2180
Loading…
x
Reference in New Issue
Block a user
No description provided.
Delete Branch "feature/ganttastic"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This PR replaces our home-grown gantt chart implementation with vue-ganttastic which is a lot more robust and seems to work better overall.
Resolves #2169
Resolves #2115
Resolves https://github.com/go-vikunja/frontend/issues/71
Hopefully Resolves #1487
Related forum discussions:
https://community.vikunja.io/t/gantt-display-wrong/316
https://community.vikunja.io/t/gantt-display-wrong-date/525
Hi konrad!
Thank you for creating a PR!
I've deployed the changes of this PR on a preview environment under this URL: https://2180-feature-ganttastic--vikunja-frontend-preview.netlify.app
You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.
Have a nice day!
Thank you very much, and this preview bot is fantastic, too!
Nitpick: "Create a new task" at the bottom only works when pressing Enter, not when pressing the button.
general usability thing:
The scrollbar should be always visible, e.g. if you go to https://2180-feature-ganttastic--vikunja-frontend-preview.netlify.app/lists/16/gantt and show undated tasks, you need to fiddle a lot to do long-term schedules on smaller screen sizes.
Also, limiting the calendar to one month is very unfit for typical project planning. Infinite zooming and panning, including sidescrolling if a task is dragged outwards would be fantastic.
I've increased the default range to more than two months now.
Did you try changing the date range?
And side-scrolling - yes, that's definitely something nice to have. It is a bit more work though so I'd like to do that in another PR.
When I'm trying to set "Size → Month", prod hangs, dev infinitely floods the browser console with this error:
Screenshot from the debugger:
@ -0,0 +1,44 @@
<!--
Please fill out this issue template to report a bug.
remove from pull request
Done.
@ -0,0 +1,341 @@
<template>
<div class="gantt-container loading-container" :class="{'is-loading': taskService.loading || taskCollectionService.loading}">
Use loading component. This way it's easier for us to refactor the
is-loading
styles from bulma later.Done.
@ -0,0 +9,4 @@
:grid="true"
@dragend-bar="updateTask"
@dblclick-bar="openTask"
font="'Open Sans', sans-serif"
Is it possible to use here simply
inherit
as value?Seems to work, yes.
Not necessary with lates release. Removed.
@ -0,0 +59,4 @@
import {format, parse} from 'date-fns'
import {colorIsDark} from '@/helpers/color/colorIsDark'
import {useStore} from 'vuex'
import Rights from '../../models/constants/rights.json'
Picky: Use
@/models...
Done.
@ -0,0 +63,4 @@
import TaskModel from '@/models/task'
import {useRouter} from 'vue-router'
const dateFormat = 'yyyy-LL-dd HH:mm'
picky: use
DATE_FORMAT
to make clear it's a 'config const'But also: shouldn't this depend on the user setting / language?
It's only used to pass the date in the correct format to the gantt chart libaray so it will always be the same. Not sure why they only take strings as input instead of
Date
objects but that's how it is.@ -0,0 +87,4 @@
},
showTasksWithoutDates: {
type: Boolean,
required: false,
picky: If you use a default value there is no need to define
required
Done.
@ -0,0 +107,4 @@
const canWrite = computed(() => store.state.currentList.maxRight > Rights.READ)
const tasks = ref([])
define types
Done.
@ -0,0 +141,4 @@
}
async function loadTasks() {
tasks.value = new Map()
define types
Done.
@ -0,0 +173,4 @@
mapGanttBars()
}
loadTasks()
This and the three lines below should be combined to one watcher that trigger immediately.
loadTasks
should accept these three as params, so that it's clear that these are needed to reload.Something like:
Done.
@ -0,0 +184,4 @@
task.startDate = e.bar.startDate
task.endDate = e.bar.endDate
const r = await taskService.value.update(task)
for (const i in ganttBars.value) {
Avoid
for ... in
, see: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in#array_iteration_and_for...inDone.
@ -0,0 +192,4 @@
}
const newTaskFieldActive = ref(false)
const newTaskTitleField = ref()
Add type: https://vuejs.org/guide/typescript/composition-api.html#typing-template-refs
Done.
@ -124,6 +124,9 @@ if (window.SENTRY_ENABLED) {
import('./sentry').then(sentry => sentry.default(app, router))
}
import ganttastic from '@infectoone/vue-ganttastic'
Don't load lib global. We only want it in the gantt view.
We could copy these lines instead
474a4fdf40/src/index.ts (L3-L14)
and simply import the two components.I tried that but the published npm package only provides a bundle. No individual components..
solved
@ -89,2 +75,4 @@
const showTasksWithoutDates = ref(false)
const precision = ref('day')
const now = ref(new Date())
Should this update?
No, doesn't even need to be ref.
@ -93,0 +80,4 @@
const defaultTo = format(new Date((new Date()).setDate(now.value.getDate() + 55)), 'yyyy-LL-dd')
const range = ref(`${defaultFrom} to ${defaultTo}`)
// TODO: only update once both dates are available (maybe use a watcher + refs instead?)
Can you explain in a different way?
The problem is the gantt chart already updates when only one date (the start or end date) is selected. Ideally, they would only update the prop when both of these dates are available to avoid these partial updates.
Maybe I'm still not getting this correctly, but can't we just update the value when both (start and end) are set?
Currently the from and to dates get passed as individual props. That means if one changes, it changes directly in the chart.
I think the way to go here would be to pass a single object with both dates instead?
That seems like the right approach
Will check this out again. Shouldn't be too hard.
@ -4607,6 +4614,11 @@ dayjs@^1.10.4:
resolved "https://registry.yarnpkg.com/dayjs/-/dayjs-1.11.3.tgz#4754eb694a624057b9ad2224b67b15d552589258"
integrity sha512-xxwlswWOlGhzgQ4TKzASQkUhqERI3egRNqgV4ScR8wlANA/A9tZ7miXa44vTTKEq5l7vWoL5G57bG3zA+Kow0A==
dayjs@^1.10.5:
We now have two date libs
Yes. Since dayjs is a small one, I think we should be fine to leave it in for now. We should make sure only one is used in the future though.
Too bad
vue-ganttastic
picked that library :DThanks for all the comments.
I'll be on vacation until 30.07., will take a proper look at all of them after that.
Nope, missed that, woops - thank you :)
Totally agree. Just to keep it in mind, in case the current implementation proves unfit for that in some way.
See #2197
More nitpicks:
When opening the view, it always scrolls two weeks to the left, even if all tasks are further ahead. Maybe encapsulate them with some buffer if there are set tasks.
https://2180-feature-ganttastic--vikunja-frontend-preview.netlify.app/lists/6/gantt
Hope you had a nice vacation :)
Can you look into getting the basic version of this merged soon? We'd love to use it for our project planning! If not, a rough ETA would be helpful, too.
I will also contact you again soon per mail about us contributing back, but currently there is too much to do here...
5442306b7c
toc8693bc07a
I looked into that and as it seems now, setting the size to "Month" does not make any sense. The size is the same, it just won't show the dates anymore. Let's remove the size dropdown for now and handle this in a follow-up PR.
@xeruf You mean checking where the first task begins and then add a small buffer to it instead of buffering two weeks every time?
Yes that's correct, thank you :)
@ -0,0 +104,4 @@
const canWrite = computed(() => store.state.currentList.maxRight > Rights.READ)
const tasks = ref<Map<number, TaskModel>>([])
User
TaskModel['id']
Done (I think you did that one?)
@ -0,0 +268,4 @@
.g-timeunit .timeunit-wrapper {
padding: 0.5rem 0;
font-size: 1rem !important;
why do we need so many overwriting important styles here?
IIRC they're using inline styles for these :/
00ff7345b7
to774f857fe6
774f857fe6
to7ab4ff2d9e
I created a pull request for vue-gantastic to avert some of the issues.
See https://github.com/InfectoOne/vue-ganttastic/pull/53
Very nice!
Is this gonna be live soon? :)
@xeruf well soon is relative - I'd say it depends on how fast we can get the pr in ganttastic merged and they release a new version which well then need to integrate back into this pr.
@konrad do you want to rebase this on main?
@dpschen Done
I finished the pull-request for vue-ganttastic. Waiting for review now.
FYI:
I'm trying to further improve the performance of vue-ganttastic. This doesn't block this pull request though and can also be amended later.
The branch is merged, new release should be out soon.
Let's just merge this and I'll add update the new version later.
@konrad we should not yet upgrade to the new v2.1.1 yet. There are still some issues.
@dpschen Ist v2.1.1 the version with your improvements?
Looks like the CI is still failing :/
Yes, there are still aome issues with the types that I like to solve. I didnt know how I could test these so these problems just appeared for me after the release,
Okay, then let's fix whatever is required to make the CI happy and then merge this.
I've now fixed everything so that the CI should work but it bothers me that we have to import all gantt components in
main.ts
.Put up an issue: https://github.com/InfectoOne/vue-ganttastic/issues/61
That was one of the things that I fixed: https://github.com/InfectoOne/vue-ganttastic/blob/master/src/index.ts#L18
@dpschen so let's upgrade?
Too be honest I think I did some mistake in exporting the types when I created the chanes in vue-ganttastic. Something still isn't working for me:
This is the branch where I tested that: https://kolaente.dev/dpschen/frontend/src/branch/feature/ganttastic-with-updated-version (not rebased)
Are the type declarations missing? Or not properly exported in the release?
I think the way I exported them was wrong. Although what was put out appeared to be correct. See https://github.com/InfectoOne/vue-ganttastic/blob/master/package.json#L10
Should we wait for fixed exports or merge this once the tests pass?
I have to check but I think the types are already exported. If that's true just the components types are missing. In that case we could use dummy component types and use the new branch already.
I currently don't know how to fix the component type export issue itself though, but also didn't look at it anymore.
I could do the stuff I wrote above if you want.
I'd appreciate that.
I got the types working:
https://github.com/InfectoOne/vue-ganttastic/pull/62
I will chceck out if I can solve this as well:
#2180 (comment)
I currently added the fixed lib as local reference. This might break the build for now (checking it out and building it local should work though). I don't want to waste time and fix that because if there will be a new version that work would be wasted anyway. What's probably needed is copying over the added local vendor folder.
There is another issue with the chart itself:
The handling of tasks without date ranges is really cumbersome. One can move them around but the start and end date won't get set. This is different and worse than it was with the old implementation.
That sounds like a bug.
See this video:
Once moved the tasks should turn blue.
9d35feb7bf
to1d495b8603
@ -0,0 +1,234 @@
<template>
What are the flatpickr changes doing in this PR?
@ -0,0 +1,21 @@
import { defineAsyncComponent, type AsyncComponentLoader, type AsyncComponentOptions, type Component, type ComponentPublicInstance } from 'vue'
Is this new helper relevant for the gantt chart?
I load the GanttChart async. This way the chart library can be loaded when necessary.
Okay I think this could be improved/simplified further by just loading the whole gantt view async as that's the only place where the chart is loaded. But that's something I'd leave for another PR.
I thought GanttChart makes more sense, since it's our custom wrapper where we also load some other stuff :) This way all additional load gets loaded async while the page load is really fast, since lightweight.
@ -0,0 +1,30 @@
import type {DateISO} from '@/types/DateISO'
import type {DateKebab} from '@/types/DateKebab'
export function parseDateProp(kebabDate: DateKebab | undefined): string | undefined {
Isn't this whole function locale-dependant?
No. What I called KebabDate and ISO dates are not locale dependant afaik.
After I created it I realized that KebabDate is actually a valid ISO date. I thought that it still makes sense to keep both types around since there are conversions in both directions.
@ -86,0 +105,4 @@
return
}
console.log('foo')
await import(`../../node_modules/dayjs/locale/${dayjsLanguageCode}.js`)
This statement did not work in my tests. IMHO we could merge the gantt chart without proper locale support and rework that later. This PR is already a lot bigger than it should be.
Agree!
I found out how to make this work. Turned out that more work is needed since after the locale support works the
weekdayFromTimeLabel
doesn't anymore. So better to add this at a later point.And it looks like the task input is now broken :/
I could fix this (not pushed yet).
Looks good! I'll rebase once more, should be ready to merge after that.
483a60452b
to61a89117d2