fix: upcoming view with datepicker and filters #1261

Closed
konrad wants to merge 0 commits from fix/upcoming into main
Owner

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

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
konrad added 19 commits 2021-12-29 20:08:46 +00:00
konrad reviewed 2021-12-29 20:10:44 +00:00
@ -0,0 +2,4 @@
<div class="datepicker-with-range-container">
<popup>
<template #trigger="{toggle}">
<a @click.prevent.stop="toggle()">
Author
Owner

I'm not quite sure if this should be a button?

I'm not quite sure if this should be a button?

Should be one!

Should be one!
Author
Owner

Changed it to a button.

Changed it to a button.
konrad marked this conversation as resolved
@ -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)
Author
Owner

I have no idea why this does not work.

I have no idea why this does not work.
Solving this here: https://kolaente.dev/vikunja/frontend/pulls/1342/files#issuecomment-25401
Author
Owner

It's not reflected in the flatpicker config (below). That always contains 0 as week start, no matter what is set in store.

It's not reflected in the flatpicker config (below). That always contains `0` as week start, no matter what is set in store.
dpschen marked this conversation as resolved
@ -0,0 +212,4 @@
position: relative;
z-index: 10;
:deep(.popup) {
Author
Owner

This feeld kind of hacky, not quite sure if there's a better way?

This feeld kind of hacky, not quite sure if there's a better way?
Author
Owner

Currently this kind of works on mobile, but it's far from ideal.

Currently this kind of works on mobile, but it's far from ideal.
konrad added 1 commit 2021-12-29 20:12:47 +00:00
continuous-integration/drone/pr Build is failing Details
294e89b6f7
fix: z-index
konrad added 1 commit 2021-12-29 20:34:00 +00:00
continuous-integration/drone/pr Build is failing Details
0710cea9e5
fix: lint
Member

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!

Beep boop, I'm a bot.

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! > Beep boop, I'm a bot.
konrad added 1 commit 2021-12-30 15:12:56 +00:00
continuous-integration/drone/pr Build is failing Details
1648bcdb70
chore: make select date button actually a button
konrad added 1 commit 2021-12-30 15:14:56 +00:00
continuous-integration/drone/pr Build is failing Details
7dddfea79e
fix: test
konrad added 1 commit 2022-01-09 10:51:05 +00:00
continuous-integration/drone/pr Build is passing Details
01323a1b45
Merge branch 'main' into fix/upcoming
# Conflicts:
#	src/views/tasks/ShowTasks.vue
konrad requested review from dpschen 2022-01-09 11:12:13 +00:00
dpschen was assigned by konrad 2022-01-09 11:12:17 +00:00
dpschen reviewed 2022-02-05 17:20:25 +00:00
@ -0,0 +79,4 @@
},
}))
const dateRange = ref<string>('')

Picky note: no need for typing if you add a default that has the type.

Picky note: no need for typing if you add a default that has the type.
konrad marked this conversation as resolved
@ -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:

<datepicker-with-range @dateChanged="setDate">
	<template #trigger="{ toggle }">
		<x-button
			@click="toggle()"
			type="secondary"
			:shadow="false"
			class="mb-2"
		>
			{{ $t('task.show.select') }}
		</x-button>
	</template>
</datepicker-with-range>
DatepickerWithRange should provide a slot for the trigger. That way it's possible to open the popup with any button we want. Something like: ```vue <datepicker-with-range @dateChanged="setDate"> <template #trigger="{ toggle }"> <x-button @click="toggle()" type="secondary" :shadow="false" class="mb-2" > {{ $t('task.show.select') }} </x-button> </template> </datepicker-with-range> ```

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.

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.
Author
Owner

Added a slot, but kept the current button as default to re-use the current code.

Added a slot, but kept the current button as default to re-use the current code.
Author
Owner

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.

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.

> 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. 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:

  • the url
  • the component

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.

By having a local state on it's own we have the state defining start date and end date at two places: - the url - the component 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.
Author
Owner

I'm not quite sure I understand. Wouldn't using v-model prevent us from using route props?

I'm not quite sure I understand. Wouldn't using v-model prevent us from using route props?
dpschen reviewed 2022-02-05 17:24:33 +00:00
@ -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?

Why do you need `.prevent.stop` here?
Author
Owner

Not quite sure, but the popup does not open if I don't have it.

Not quite sure, but the popup does not open if I don't have it.

This seems like an issue with the popup code

This seems like an issue with the popup code
konrad marked this conversation as resolved
dpschen reviewed 2022-02-05 17:31:16 +00:00
@ -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:

const now = newDate()
const startDate = startOfDay(new Date(now))
const endDate = endOfDay(new Date(now))
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: ```js const now = newDate() const startDate = startOfDay(new Date(now)) const endDate = endOfDay(new Date(now)) ```
Author
Owner

Looks like it is actually almost twice as fast to use new Date(now) instead of doing a new 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.

Looks like it is actually almost twice as fast to use `new Date(now)` instead of doing a `new 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.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-05 17:35:31 +00:00
@ -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()

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()
Author
Owner

Will be obsolete with #1342

Will be obsolete with #1342
konrad marked this conversation as resolved
dpschen reviewed 2022-02-05 17:37:25 +00:00
@ -0,0 +105,4 @@
return format(date, 'yyyy-MM-dd HH:mm')
}
function startOfDay(date: Date): Date {

"date" should be cloned to stay functional.

"date" should be cloned to stay functional.
Author
Owner

Will be obsolete with #1342

Will be obsolete with #1342
konrad marked this conversation as resolved
dpschen reviewed 2022-02-05 17:37:35 +00:00
@ -0,0 +112,4 @@
return date
}
function endOfDay(date: Date): Date {

"date" should be cloned to stay functional.

"date" should be cloned to stay functional.
Author
Owner

Will be obsolete with #1342

Will be obsolete with #1342
konrad marked this conversation as resolved
dpschen reviewed 2022-02-05 17:44:03 +00:00
@ -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):

const customRanges = reactive({
	datesToday,
    datesThisWeek,
    datesNextWeek,
    datesNext7Days,
    datesThisMonth,
    datesNextMonth,
    datesNext30Days,
})

// really unsure here if the .value in range.value is necessary
const customRangeActive = computed(() => Object.values(customRanges).every((range) => range.value !== dateRange.value))
You can set a ref inside a reactive and it stays reactive. That means you could do something like (not tested): ```js const customRanges = reactive({ datesToday, datesThisWeek, datesNextWeek, datesNext7Days, datesThisMonth, datesNextMonth, datesNext30Days, }) // really unsure here if the .value in range.value is necessary const customRangeActive = computed(() => Object.values(customRanges).every((range) => range.value !== dateRange.value)) ```
Author
Owner

I did something similar in #1342, so this here will become obsolete as well.

I did something similar in #1342, so this here will become obsolete as well.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-05 17:50:02 +00:00
@ -92,8 +67,6 @@ export default {
showAll: Boolean,
},
created() {
this.cStartDate = this.startDate

Removing this breaks the showTasksInRange component.

Removing this breaks the showTasksInRange component.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-05 17:52:44 +00:00
@ -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 =)

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:

		{
			path: '/tasks/:id',
			name: 'task.detail',
			component: TaskDetailView,
			props: route => ({ taskId: parseInt(route.params.id as string) }),
		},
Is used this already for the task detail: ```js { path: '/tasks/:id', name: 'task.detail', component: TaskDetailView, props: route => ({ taskId: parseInt(route.params.id as string) }), }, ```
Author
Owner

I've moved the logic of the ShowTasksInRange component to ShowTasks and removed the former.

Will look into route props.

I've moved the logic of the `ShowTasksInRange` component to `ShowTasks` and removed the former. Will look into route props.
Author
Owner

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.

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.
Author
Owner

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.

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).

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).
konrad marked this conversation as resolved
dpschen reviewed 2022-02-05 17:55:22 +00:00
@ -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

Use `v-else`
konrad marked this conversation as resolved
dpschen reviewed 2022-02-05 18:00:34 +00:00
@ -269,2 +223,2 @@
vertical-align: middle;
margin: .5rem 0;
> :deep(a) {
margin-right: .5rem;

What is styled with this?

What is styled with this?
Author
Owner

Looks like nothing styles it, will remove.

Looks like nothing styles it, will remove.

Maybe this broke with the addition of the BaseButton

Maybe this broke with the addition of the BaseButton
Author
Owner

Maybe. I didn't notice anything that wasn't properly styled without the rule so I think it should be fine.

Maybe. I didn't notice anything that wasn't properly styled without the rule so I think it should be fine.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-05 18:06:23 +00:00
@ -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.

const loadPendingTasksParameter = computed(() => ({
	propA: route.query.propA || 'fallback',
    // [...]
})

watchEffect(() => loadPendingTasks(loadPendingTasksParameter.value))

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.

Instead of watching the whole route object for changes it's better to build a computed with the needed props. ```js const loadPendingTasksParameter = computed(() => ({ propA: route.query.propA || 'fallback', // [...] }) watchEffect(() => loadPendingTasks(loadPendingTasksParameter.value)) ``` 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.
Author
Owner

ah so in our case we should be able to just reuse the dateFrom and dateTo components as watching?

ah so in our case we should be able to just reuse the `dateFrom` and `dateTo` 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 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.
Author
Owner

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?

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?
dpschen reviewed 2022-02-05 18:06:58 +00:00
@ -56,33 +38,26 @@
</div>
</template>
<script>

Use script setup / use ts

Use script setup / use ts
Author
Owner

I'd like to do that in #1342 since I've already modified a bunch of stuff in there.

I'd like to do that in #1342 since I've already modified a bunch of stuff in there.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-05 18:10:15 +00:00
@ -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.

If datepickerWithRange would use a v-model we wouldn't need a fallback here.
Author
Owner

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...

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...
konrad marked this conversation as resolved
konrad added 4 commits 2022-02-05 19:19:37 +00:00
konrad added 1 commit 2022-02-05 19:30:02 +00:00
konrad added 1 commit 2022-02-05 20:12:46 +00:00
konrad added 1 commit 2022-02-05 20:14:43 +00:00
konrad added 1 commit 2022-02-05 20:41:26 +00:00
konrad added 1 commit 2022-02-06 11:45:21 +00:00
continuous-integration/drone/pr Build is passing Details
1abd36ef6e
Merge branch 'main' into fix/upcoming
Member

Currently this kind of works on mobile, but it's far from ideal.

Could you improve this in the meantime?

> Currently this kind of works on mobile, but it's far from ideal. Could you improve this in the meantime?
Author
Owner

Closing this in favor of #1342

Closing this in favor of #1342
konrad closed this pull request 2022-02-06 14:30:13 +00:00
dpschen refused to review 2022-02-06 21:38:34 +00:00
dpschen removed their assignment 2022-02-06 21:38:43 +00:00
This repo is archived. You cannot comment on pull requests.
No description provided.