feat: add date math for filters #1342
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#1342
Loading…
Reference in New Issue
No description provided.
Delete Branch "feature/date-math"
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 adds support for date math to all filter areas.
Depends on #1261
Frontend for vikunja/api#1086
@ -0,0 +1,12 @@
export function parseDateOrString(rawValue: string, fallback: any) {
Maybe we should just move this to the emits section of the date range component? To avoid callees requiring to import it every time?
Yes that makes sense!
Since we're now using route props where we need this function I think we'll have to leave it as is.
Hi konrad!
Thank you for creating a PR!
I've deployed the changes of this PR on a preview environment under this URL: https://1342-featuredate-math--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!
2e66e83c9c
to9e7c258347
@ -0,0 +3,4 @@
<popup>
<template #trigger="{toggle}">
<slot name="trigger" :toggle="toggle">
<x-button @click.prevent.stop="toggle()" variant="secondary" :shadow="false" class="mb-2">
The styles (=props) selected for the button are hightly specific and create an indirect dependency to its indented use case. The latter might change in the future and then we have to remember this dependency. This is why I think it makes sense to remove at least the specific use case of this button. Since we wouldn't use it then at all anymore (since you need to overwrite the default slot) it might make it simpler to only define the slot.
Moved everything to a slot.
@ -0,0 +11,4 @@
<template #content="{isOpen}">
<div class="datepicker-with-range" :class="{'is-open': isOpen}">
<div class="selections">
<button @click="setDateRange(null)" :class="{'is-active': customRangeActive}">
Use BaseButton
Done.
@ -0,0 +14,4 @@
<button @click="setDateRange(null)" :class="{'is-active': customRangeActive}">
{{ $t('misc.custom') }}
</button>
<button
Use BaseButton
Done.
@ -0,0 +27,4 @@
{{ $t('input.datepickerRange.from') }}
<div class="field has-addons">
<div class="control is-fullwidth">
<input class="input" type="text" v-model="from" @change="inputChanged"/>
Instead of calling
inputChanged
from here better watch thefrom
and below theto
value.This prevents future mistakes
Changed. I kept
inputChanged
though because I need to watchers.@ -0,0 +52,4 @@
<p>
{{ $t('input.datepickerRange.math.canuse') }}
<a @click="showHowItWorks = true">{{ $t('input.datepickerRange.math.learnhow') }}</a>.
Use BaseButton
Done.
@ -0,0 +73,4 @@
</i18n-t>
</p>
<p>
<i18n-t keypath="input.datepickerRange.math.similar">
I think it's really cool, that we support this, but it would be cool, if we could break this down and explain it in our / simpler terms.
I think I'm already doing that but wanted to highlight for people who know the syntax from Elasticsearch or Grafana they can use it here as well.
Was there anything in the explanation you noticed as not quite understandable?
I didn't even check to be honest.
Was just something I though of while reading this.
@ -0,0 +62,4 @@
:overflow="true"
variant="hint-modal"
>
<card class="has-no-shadow how-it-works-modal" :title="$t('input.datepickerRange.math.title')">
The whole explanation card should be its own component.
Done!
@ -0,0 +173,4 @@
</template>
<script lang="ts" setup>
import flatPickr from 'vue-flatpickr-component'
Picky:
I like to group imports from external packages and from internal stuff.
Usally I follow this order (but not strict):
external packages beginning with vue libs
external packages other libs and css
internal instances like store, message, etc
services and models
component imports
helpers
As you can see this is more or less random, so happy to discuss this.
Or we can simple keep it random :D
Oh I think it absolutely makes sense to have something like that, even if not strictly enforced. Do you think we could put this in a linter?
I think I encountered at least something like external stuff first in a linter already.
@ -0,0 +177,4 @@
import 'flatpickr/dist/flatpickr.css'
import {computed, ref, watch} from 'vue'
import {useI18n} from 'vue-i18n'
import {store} from '@/store'
After using it a while I prefer now to use the
useStore
method to get the current store instance.The reason is that it makes it easier to refactor in composables later because
useStore
will always give you the store.Makes sense! Changed it.
@ -0,0 +1,21 @@
export const dateRanges = {
Use UPPER_SNAKE_CASE for constants
Done.
@ -0,0 +213,4 @@
const flatpickrRange = ref('')
const from = ref('')
I think this is a good usecase for a reactive.
What would be the advantage over a ref?
Both values can be changed at the same time
=> watchers aren't triggered twice when you change stuff shortly after each other.
How do I type it properly? If I only pass a string into a reactive it complains I should pass in an object instead.
Edit: using
String
(capital S) seems to work.Okay even with the typing solved it still complains when trying to set it: Because we do
const from = ...
I won't be able to dofrom = fromDate
. Any idea how to solve this?@ -0,0 +263,4 @@
inputChanged()
}
const customRangeActive = computed<Boolean>(() => {
Not 100% sure but I think unnecesary: Adding the
<Boolean>
as definition here.Regardless I think it should be
<boolean>
(lowercase "b")Changed.
@ -0,0 +283,4 @@
.datepicker-with-range-container {
position: relative;
:deep(.popup) {
No need for nesting
Done!
@ -0,0 +304,4 @@
height: 100%;
position: absolute;
:deep(.flatpickr-calendar) {
No need for nesting
Done
@ -0,0 +370,4 @@
font-size: 1rem;
p {
display: inline-block !important;
Why the
!important
Very good question. It looks like this style wasn't applied anyway so I removed it.
@ -87,4 +68,3 @@
}
},
props: {
startDate: Date,
Readd route props
Changed it to route props.
However, we still have a dependency on the router: every time when seleting a date in ShowTasks it will push the change to the route. I don't know how to change that without massively overengeneering everything so I'd say we leave it at that.
I think it's fine for now. I thought a while about this but also don't have a better soltion for this at the moment (except the v-model version)
@ -123,2 +86,2 @@
firstDayOfWeek: this.$store.state.auth.settings.weekStart,
},
dateFrom() {
return parseDateOrString(this.$route.query.from, new Date())
get value from props to remove dependency on route. Removing dependency from router makes the components easier reusable nested inside another view (which is what we do).
@ -125,1 +112,4 @@
})
}
this.setTitle(title)
This is a sideeffect inside a computed. Remove this.
Better watch the computed and react to that:
Done.
@ -213,3 +212,1 @@
? b.id - a.id
: sortByDueDate
})
this.tasks = await this.$store.dispatch('tasks/loadTasks', params)
Instead of creating the params already in snake_case:
Wait until the last moment (ideally abstracted away from a apiClient) until you convert params from camelCase to snake_case, needed by the api. This way we use consequently use camelCase in most areas of the frontend – which is what you usually do in JS.
I agree. However, this whole filter thing is a mess right now, not only here. I'd like to fix this everywhere at once at some point, simplifying the filter handling in the process as well.
That being said, I've changed there here for now.
@ -0,0 +194,4 @@
},
})
// FIXME: This seems to always contain the default value - that breaks the picker
Continuing from #1261 (comment)
With default value you mean the value of store.state.auth.settings.weekStart?
As said in #1261:
@ -175,15 +151,14 @@
</template>
<script>
Use script setup and ts
This component's js is ~450 lines, I'm not sure this PR is a good place to refactor it.
If we don't start converting when we touch the component, then when will we?
As a compromise we could try to add
lang="ts"
on these occasions and add everything new inside a new setup function =)yeah well I can't say anything against that :) I keep saying myself "yeah we'll do that one day" but I think we need to put in the effort.
@ -0,0 +2,4 @@
<div class="datepicker-with-range-container">
<popup>
<template #trigger="{toggle}">
<slot name="trigger" :toggle="toggle">
Provide
buttonText
as slot prop aswellDone.
@ -0,0 +188,4 @@
const emit = defineEmits(['dateChanged'])
const props = defineProps({
showSelectedOnButton: {
We could remove this prop if we would only expose the slot for the trigger.
Done.
@ -0,0 +217,4 @@
const to = ref('')
function emitChanged() {
emit('dateChanged', {
Use v-model for values.
Might be useful: https://vueuse.org/core/usevmodel/
What I don't like with using a v-model here would be the need to introduce another watcher in ShowTasks because we need to push the changes to the route. I think the current way with an event nicely circumvents this.
Also I'd like to keep the current behaviour where it only does one route change if you change both values instead of two (but that could easily be kept with passing an object to v-model?).
correct
WIP: feat: add date math for filtersto feat: add date math for filters@ -0,0 +13,4 @@
</p>
<p>
<i18n-t keypath="input.datepickerRange.math.similar">
<a
use BaseButton
Done.
@ -0,0 +18,4 @@
rel="noreferrer noopener nofollow" target="_blank">
Grafana
</a>
<a
use BaseButton
@ -0,0 +116,4 @@
}
function inputChanged() {
flatpickrRange.value = ''
Why do we need to reset this every time?
I don't think we have to. I've checked and it looks like this doesn't really break anything so I've removed it. Lets us get rid of
inputChanged
.@ -0,0 +117,4 @@
function inputChanged() {
flatpickrRange.value = ''
emitChanged()
Called here!
@ -0,0 +136,4 @@
from.value = fromDate
to.value = toDate
emitChanged()
This is triggered three times!
Once for each of these lines:
in the called function inputChanged of the watcher.
And then here again.
mhh how can we fix this? Using a debounce to prevent it getting called three times if it changed all values at once? Or removing the watchers again?
It looks like the tasks are only loaded once, not three times. Maybe that's already enough?
@ -0,0 +155,4 @@
}
const customRangeActive = computed<boolean>(() => {
return !Object.values(DATE_RANGES).some(el => from.value === el[0] && to.value === el[1])
picky:
el
is a bit misleading here. We don't have elementsright, I've changed it.
@ -0,0 +257,4 @@
}
}
.how-it-works-modal {
This style is in the wrong component
Moved it to the helper.
@ -46,2 +46,4 @@
export function objectToSnakeCase(object) {
if (object instanceof Date) {
return object.toISOString()
Why using here
toISOString()
and not just return the object?Because if I pass the object, it gets serialized as
{}
which is not what we want. This might be the wrong place to do that though.I think we could move the entire conversion to the service?
Yes that makes sense
Done.
@ -0,0 +15,4 @@
<i18n-t keypath="input.datepickerRange.math.similar">
<BaseButton
href="https://grafana.com/docs/grafana/latest/dashboards/time-range-controls/"
rel="noreferrer noopener nofollow" target="_blank">
Remove rel attribute here and below.
Show we also add
target="_blank"
as default for the BaseButton is a link case?Mh I think there will still be cases where we want to control this so right now I think we should not add one.
@ -0,0 +3,4 @@
class="has-no-shadow how-it-works-modal"
:title="$t('input.datepickerRange.math.title')">
<p>
{{ $t('input.datepickerRange.math.intro') }}
The naming of this feels wrong now that it's not part of the component anymore.
If this help is specific to datepickerRange maybe we should also show that via the component name.
Changed!
@ -530,3 +500,2 @@
},
setReminderFilter() {
this.setDateFilter('reminders')
setDueDateFilter(values) {
These abstractions are unnecessary and just increase complexity, call
setDateFilter
directly, like:@dateChanged="(values) => setDateFilter('due_date', values)"
Done.
@ -142,0 +72,4 @@
setTimeout(() => showNothingToDo.value = true, 100)
// NOTE: You MUST provide either dateFrom and dateTo OR showAll for the component to actually show tasks.
Wouldn't it be better to make showAll a computed that gets autoset when dateFrom and dateTo doesn't contain a value?
Excellent idea. Changed it!
@ -142,0 +94,4 @@
// We need to define "key" because it is the first parameter in the array and we need the second
// eslint-disable-next-line no-unused-vars
const predefinedRange = Object.entries(DATE_RANGES).find(([key, value]) => dateFrom === value[0] && dateTo === value[1])
Unsure: I think eslint doesn't complain here if you use underscore (
_
) to spread the unused variable.Add
?.[0]
so we return the key, which is what we actually want to use and adjust variable name.Nope, does not seem to have an effect.
Done.
Interesting!
@ -142,0 +95,4 @@
// We need to define "key" because it is the first parameter in the array and we need the second
// eslint-disable-next-line no-unused-vars
const predefinedRange = Object.entries(DATE_RANGES).find(([key, value]) => dateFrom === value[0] && dateTo === value[1])
if (typeof predefinedRange !== 'undefined') {
Remove title var and return directly:
Done.
@ -216,0 +145,4 @@
query: {
from: dates.dateFrom ?? dateFrom,
to: dates.dateTo ?? dateTo,
showOverdue: showOverdue ? 'true' : 'false',
Mhh that is a bit annoying that we have to convert to strings here :/
But I also don't have a better idea for now.
Right now this might be still fine, but I remember similar usecases where the conversion from state object to the url representation was quite complex.
We might need to abstract this in the future, so let's keep that in the back of our head =)
@ -245,3 +174,1 @@
this.showOverdue = false
this.setDate()
},
async function loadPendingTasks(from: string, to: string) {
This should be marked as a Hack because it's something that shouldn't happen.
When I tried to change the auth these days I tried to fix this, but got tangled.
We should try to solve this in the future because else we will just add more and more hacks =)
Done.
@dpschen What's left to do here?