fix: upcoming view with datepicker and filters #1261
No reviewers
Labels
No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
No Milestone
No project
No Assignees
3 Participants
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/frontend#1261
Loading…
Reference in New Issue
No description provided.
Delete Branch "fix/upcoming"
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 improves the "upcoming" view. There's now a proper way to pick a date and the filters (show tasks without dates, show overdue) now actually work.
Relevant community discussion: https://community.vikunja.io/t/question-about-showing-overdue-tasks-in-upcoming-views/501/5
@ -0,0 +2,4 @@
<div class="datepicker-with-range-container">
<popup>
<template #trigger="{toggle}">
<a @click.prevent.stop="toggle()">
I'm not quite sure if this should be a button?
Should be one!
Changed it to a button.
@ -0,0 +66,4 @@
const emit = defineEmits(['dateChanged'])
// FIXME: This seems to always contain the default value - that breaks the picker
const weekStart = computed<number>(() => store.state.auth.settings.weekStart ?? 0)
I have no idea why this does not work.
Solving this here: https://kolaente.dev/vikunja/frontend/pulls/1342/files#issuecomment-25401
It's not reflected in the flatpicker config (below). That always contains
0
as week start, no matter what is set in store.@ -0,0 +212,4 @@
position: relative;
z-index: 10;
:deep(.popup) {
This feeld kind of hacky, not quite sure if there's a better way?
Currently this kind of works on mobile, but it's far from ideal.
Hi konrad!
Thank you for creating a PR!
I've deployed the changes of this PR on a preview environment under this URL: https://1261-fixupcoming--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!
@ -0,0 +79,4 @@
},
}))
const dateRange = ref<string>('')
Picky note: no need for typing if you add a default that has the type.
@ -37,2 +6,2 @@
<x-button variant="secondary" @click="setDatesToNextMonth()">{{ $t('task.show.nextMonth') }}</x-button>
</div>
<p v-if="!showAll" class="show-tasks-options">
<datepicker-with-range @dateChanged="setDate"/>
DatepickerWithRange should provide a slot for the trigger.
That way it's possible to open the popup with any button we want.
Something like:
It also seems to me that the datepicker-with-range should get the current date-range as a v-model.
That way it is always up to date.
Added a slot, but kept the current button as default to re-use the current code.
But given the usecase, does the component actually need to know if the outside wants to tell it a date? IMHO it's enough if it can emit new dates as they are selected in the component.
By having a local state on it's own we have the state defining start date and end date at two places:
This makes things much more complicated in the long run.
As a rule of thumb: components that have state defined from the route shouldn't keep that state separate.
I'm not quite sure I understand. Wouldn't using v-model prevent us from using route props?
@ -0,0 +2,4 @@
<div class="datepicker-with-range-container">
<popup>
<template #trigger="{toggle}">
<x-button @click.prevent.stop="toggle()" type="secondary" :shadow="false" class="mb-2">
Why do you need
.prevent.stop
here?Not quite sure, but the popup does not open if I don't have it.
This seems like an issue with the popup code
@ -0,0 +119,4 @@
return date
}
const datesToday = computed<string>(() => {
Unsure:
In this component there are a lot of positions where
new Date()
is used. All of them are initially created.Computers are fast, but I think some time will pass in between those calls.
I think it might make sense to create a new variable now for these cases.
E.g. here:
Looks like it is actually almost twice as fast to use
new Date(now)
instead of doing anew Date()
every time. Very interesting I thought the difference was neglegible.Benchmark: https://jsbench.me/jckza7okr7/1
That being said all of these functions calculating the date intervals in the next few days/weeks/whatever will become obsolete with #1342 so I don't think it makes much sense to change them here.
@ -0,0 +139,4 @@
}
const datesThisWeek = computed<string>(() => {
const {startDate, endDate} = thisWeek()
Since no reactive value is passed to thisWeek the result will not be reactive. For stuff like this the useNow of vueuse is really nice.
This is true for all computeds here that also just call new Date().
The result won't be reactive, because there is no timer that recalls new Date()
Will be obsolete with #1342
@ -0,0 +105,4 @@
return format(date, 'yyyy-MM-dd HH:mm')
}
function startOfDay(date: Date): Date {
"date" should be cloned to stay functional.
Will be obsolete with #1342
@ -0,0 +112,4 @@
return date
}
function endOfDay(date: Date): Date {
"date" should be cloned to stay functional.
Will be obsolete with #1342
@ -0,0 +195,4 @@
dateRange.value = range
}
const customRangeActive = computed<Boolean>(() => {
You can set a ref inside a reactive and it stays reactive.
That means you could do something like (not tested):
I did something similar in #1342, so this here will become obsolete as well.
@ -92,8 +67,6 @@ export default {
showAll: Boolean,
},
created() {
this.cStartDate = this.startDate
Removing this breaks the showTasksInRange component.
@ -123,3 +82,1 @@
firstDayOfWeek: this.$store.state.auth.settings.weekStart,
},
}
dateFrom() {
Instead of reading this out directly from the route we could use use vue route props:
https://next.router.vuejs.org/guide/essentials/passing-props.html#function-mode
This way the component will now have a direct dependency to the router and can still be easily wrapped by ShowTasksInRange.
Even better:
ShowTasksInRange could be removed and instead the logic inside be moved in the route definition inside the function mode route props =)
Is used this already for the task detail:
I've moved the logic of the
ShowTasksInRange
component toShowTasks
and removed the former.Will look into route props.
Okay so I think we can't really use route props here because the Home component also imports
ShowTasks
so we can't just set the props in the route instead.Not quite sure though? My attempt was to move the fallback logic in the computed properties to route props but that won't work when importing the component directly? So we need to have the fallback logic either way in
ShowTasks
I think.I wanted to move the logic inside the
ShowTasksInRange
inside the route props definition of the route, not inside the component!This of course isn't compatible with using the component then on it's own (e.g. in home as you said).
@ -46,1 +20,3 @@
<div class="tasks">
<div v-if="!hasTasks" :class="{ 'is-loading': loading}" class="spinner"></div>
<card
v-if="hasTasks"
Use
v-else
@ -269,2 +223,2 @@
vertical-align: middle;
margin: .5rem 0;
> :deep(a) {
margin-right: .5rem;
What is styled with this?
Looks like nothing styles it, will remove.
Maybe this broke with the addition of the BaseButton
Maybe. I didn't notice anything that wasn't properly styled without the rule so I think it should be fine.
@ -104,25 +77,54 @@ export default {
handler: 'loadPendingTasks',
Instead of watching the whole route object for changes it's better to build a computed with the needed props.
This way the overhead is much smaller.
E.g. in case we later add hash navigation somewhere in a modal this might always retrigger a reload.
ah so in our case we should be able to just reuse the
dateFrom
anddateTo
components as watching?I think I didn't understand you comment, but regardless: if we use route props for ShowTasks dateFrom and dateTo will simply be component props, so you would need to create a computed an watch that either way.
I started changing this with
watchEffect
but then realized I would need to refactor the whole component to use the composition api exclusively - I wanted to do that anyway but in #1342 to avoid having to deal with too many conflicts.Let's move this change over there as well?
@ -56,33 +38,26 @@
</div>
</template>
<script>
Use script setup / use ts
I'd like to do that in #1342 since I've already modified a bunch of stuff in there.
@ -136,3 +138,2 @@
query: {
from: +new Date(this.cStartDate),
to: +new Date(this.cEndDate),
from: +new Date(dateFrom ?? this.dateFrom),
If datepickerWithRange would use a v-model we wouldn't need a fallback here.
But we'd still need to check and fall back on some value if the one provided from
v-model
is not present or correct?This too has changed in #1342...
Could you improve this in the meantime?
Closing this in favor of #1342