feat: add date math for filters #1342

Merged
konrad merged 88 commits from feature/date-math into main 2022-03-28 17:30:43 +00:00
Owner

This PR adds support for date math to all filter areas.

Depends on #1261

Frontend for vikunja/api#1086

This PR adds support for date math to all filter areas. Depends on https://kolaente.dev/vikunja/frontend/pulls/1261 Frontend for https://kolaente.dev/vikunja/api/pulls/1086
konrad added 38 commits 2022-01-09 16:30:11 +00:00
continuous-integration/drone/pr Build is failing Details
75cbc73b33
fix: loading spinner
continuous-integration/drone/pr Build is failing Details
294e89b6f7
fix: z-index
continuous-integration/drone/pr Build is failing Details
0710cea9e5
fix: lint
continuous-integration/drone/pr Build is failing Details
1648bcdb70
chore: make select date button actually a button
continuous-integration/drone/pr Build is failing Details
7dddfea79e
fix: test
continuous-integration/drone/pr Build is passing Details
01323a1b45
Merge branch 'main' into fix/upcoming
# Conflicts:
#	src/views/tasks/ShowTasks.vue
continuous-integration/drone/pr Build is failing Details
3b73925de5
fix: lint
konrad reviewed 2022-01-09 16:31:50 +00:00
@ -0,0 +1,12 @@
export function parseDateOrString(rawValue: string, fallback: any) {
Author
Owner

Maybe we should just move this to the emits section of the date range component? To avoid callees requiring to import it every time?

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!

Yes that makes sense!
Author
Owner

Since we're now using route props where we need this function I think we'll have to leave it as is.

Since we're now using route props where we need this function I think we'll have to leave it as is.
konrad marked this conversation as resolved
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://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!

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://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! > Beep boop, I'm a bot.
konrad added 2 commits 2022-01-30 11:01:42 +00:00
konrad force-pushed feature/date-math from 2e66e83c9c to 9e7c258347 2022-02-06 11:43:50 +00:00 Compare
dpschen reviewed 2022-02-06 13:10:28 +00:00
@ -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.

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

Moved everything to a slot.

Moved everything to a slot.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 13:10:52 +00:00
@ -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

Use BaseButton
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 13:11:05 +00:00
@ -0,0 +14,4 @@
<button @click="setDateRange(null)" :class="{'is-active': customRangeActive}">
{{ $t('misc.custom') }}
</button>
<button

Use BaseButton

Use BaseButton
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 13:12:57 +00:00
@ -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 the from and below the to value.
This prevents future mistakes

Instead of calling `inputChanged` from here better watch the `from` and below the `to` value. This prevents future mistakes
Author
Owner

Changed. I kept inputChanged though because I need to watchers.

Changed. I kept `inputChanged` though because I need to watchers.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 13:14:02 +00:00
@ -0,0 +52,4 @@
<p>
{{ $t('input.datepickerRange.math.canuse') }}
<a @click="showHowItWorks = true">{{ $t('input.datepickerRange.math.learnhow') }}</a>.

Use BaseButton

Use BaseButton
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 13:15:42 +00:00
@ -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 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.
Author
Owner

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

I didn't even check to be honest. Was just something I though of while reading this.
dpschen marked this conversation as resolved
dpschen reviewed 2022-02-06 13:16:24 +00:00
@ -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.

The whole explanation card should be its own component.
Author
Owner

Done!

Done!
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 13:20:27 +00:00
@ -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

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

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?

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.

I think I encountered at least something like external stuff first in a linter already.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 13:21:28 +00:00
@ -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.

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

Makes sense! Changed it.

Makes sense! Changed it.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 13:22:14 +00:00
@ -0,0 +1,21 @@
export const dateRanges = {

Use UPPER_SNAKE_CASE for constants

Use UPPER_SNAKE_CASE for constants
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 13:25:25 +00:00
@ -0,0 +213,4 @@
const flatpickrRange = ref('')
const from = ref('')

I think this is a good usecase for a reactive.

I think this is a good usecase for a reactive.
Author
Owner

What would be the advantage over a ref?

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.

Both values can be changed at the same time => watchers aren't triggered twice when you change stuff shortly after each other.
Author
Owner

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.

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

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 do from = fromDate. Any idea how to solve this?

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 do `from = fromDate`. Any idea how to solve this?
dpschen reviewed 2022-02-06 13:27:18 +00:00
@ -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")

Not 100% sure but I think unnecesary: Adding the `<Boolean>` as definition here. Regardless I think it should be `<boolean>` (lowercase "b")
Author
Owner

Changed.

Changed.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 13:28:17 +00:00
@ -0,0 +283,4 @@
.datepicker-with-range-container {
position: relative;
:deep(.popup) {

No need for nesting

No need for nesting
Author
Owner

Done!

Done!
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 13:28:41 +00:00
@ -0,0 +304,4 @@
height: 100%;
position: absolute;
:deep(.flatpickr-calendar) {

No need for nesting

No need for nesting
Author
Owner

Done

Done
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 13:29:33 +00:00
@ -0,0 +370,4 @@
font-size: 1rem;
p {
display: inline-block !important;

Why the !important

Why the `!important`
Author
Owner

Very good question. It looks like this style wasn't applied anyway so I removed it.

Very good question. It looks like this style wasn't applied anyway so I removed it.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 13:32:29 +00:00
@ -87,4 +68,3 @@
}
},
props: {
startDate: Date,

Readd route props

Readd route props
Author
Owner

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.

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)

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)
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 13:33:41 +00:00
@ -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).

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).
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 13:35:46 +00:00
@ -125,1 +112,4 @@
})
}
this.setTitle(title)

This is a sideeffect inside a computed. Remove this.
Better watch the computed and react to that:

watchEffect(() => setTitle(pageTitle.value))
This is a sideeffect inside a computed. Remove this. Better watch the computed and react to that: ```js watchEffect(() => setTitle(pageTitle.value)) ```
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 13:41:41 +00:00
@ -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.

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

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.

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.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 13:43:30 +00:00
@ -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?

Continuing from https://kolaente.dev/vikunja/frontend/pulls/1261#issuecomment-22675 With default value you mean the value of store.state.auth.settings.weekStart?
Author
Owner

As said in #1261:

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

As said in #1261: > It's not reflected in the flatpicker config (below). That always contains `0` as week start, no matter what is set in store.
dpschen reviewed 2022-02-06 13:46:59 +00:00
@ -175,15 +151,14 @@
</template>
<script>

Use script setup and ts

Use script setup and ts
Author
Owner

This component's js is ~450 lines, I'm not sure this PR is a good place to refactor it.

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

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

If we don't start converting when we touch the component, then when will we?

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.

> If we don't start converting when we touch the component, then when will we? 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.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 13:48:51 +00:00
@ -0,0 +2,4 @@
<div class="datepicker-with-range-container">
<popup>
<template #trigger="{toggle}">
<slot name="trigger" :toggle="toggle">

Provide buttonText as slot prop aswell

Provide `buttonText` as slot prop aswell
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 13:50:28 +00:00
@ -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.

We could remove this prop if we would only expose the slot for the trigger.
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 13:52:00 +00:00
@ -0,0 +217,4 @@
const to = ref('')
function emitChanged() {
emit('dateChanged', {

Use v-model for values.
Might be useful: https://vueuse.org/core/usevmodel/

Use v-model for values. Might be useful: https://vueuse.org/core/usevmodel/
Author
Owner

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

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

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

> 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
konrad changed title from WIP: feat: add date math for filters to feat: add date math for filters 2022-02-06 14:30:34 +00:00
konrad added 5 commits 2022-02-06 15:04:59 +00:00
konrad added 1 commit 2022-02-06 15:46:30 +00:00
continuous-integration/drone/pr Build is failing Details
cbbcb7ef23
fix: setTitle import
konrad added 2 commits 2022-02-06 15:58:27 +00:00
konrad added 1 commit 2022-02-06 17:47:03 +00:00
continuous-integration/drone/pr Build is failing Details
c5d598cac4
chore: refactor trigger to slot
konrad added 1 commit 2022-02-06 17:51:56 +00:00
continuous-integration/drone/pr Build is failing Details
18f7adf420
chore: use more BaseButtons
konrad added 1 commit 2022-02-06 17:56:48 +00:00
continuous-integration/drone/pr Build is failing Details
204136266f
chore: watch values instead of listening to changes
konrad added 1 commit 2022-02-06 18:29:24 +00:00
continuous-integration/drone/pr Build is failing Details
eefe6bd413
chore: move date math explanation to separate component
konrad added 1 commit 2022-02-06 18:31:51 +00:00
continuous-integration/drone/pr Build is failing Details
f435ca99f4
chore: change import order and useStore
konrad added 1 commit 2022-02-06 18:33:55 +00:00
continuous-integration/drone/pr Build is failing Details
60be8b428e
chore: rename date ranges export
konrad added 1 commit 2022-02-06 18:35:33 +00:00
continuous-integration/drone/pr Build is failing Details
356b291a57
chore: change return
konrad added 2 commits 2022-02-06 18:39:13 +00:00
konrad added 2 commits 2022-02-06 18:42:03 +00:00
konrad added 1 commit 2022-02-06 19:11:17 +00:00
konrad added 1 commit 2022-02-06 19:32:24 +00:00
continuous-integration/drone/pr Build was killed Details
aac777e286
fix: lint
konrad added 1 commit 2022-02-06 19:48:40 +00:00
dpschen reviewed 2022-02-06 21:45:23 +00:00
@ -0,0 +13,4 @@
</p>
<p>
<i18n-t keypath="input.datepickerRange.math.similar">
<a

use BaseButton

use BaseButton
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 21:45:29 +00:00
@ -0,0 +18,4 @@
rel="noreferrer noopener nofollow" target="_blank">
Grafana
</a>
<a

use BaseButton

use BaseButton
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 21:52:41 +00:00
@ -0,0 +116,4 @@
}
function inputChanged() {
flatpickrRange.value = ''

Why do we need to reset this every time?

Why do we need to reset this every time?
Author
Owner

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.

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`.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 21:55:18 +00:00
@ -0,0 +117,4 @@
function inputChanged() {
flatpickrRange.value = ''
emitChanged()

Called here!

Called here!
dpschen reviewed 2022-02-06 21:55:58 +00:00
@ -0,0 +136,4 @@
from.value = fromDate
to.value = toDate
emitChanged()

This is triggered three times!

Once for each of these lines:

from.value = fromDate
to.value = toDate

in the called function inputChanged of the watcher.
And then here again.

This is triggered three times! Once for each of these lines: ```js from.value = fromDate to.value = toDate ``` in the called function inputChanged of the watcher. And then here again.
Author
Owner

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?

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?
dpschen reviewed 2022-02-06 21:58:24 +00:00
@ -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 elements

picky: `el` is a bit misleading here. We don't have elements
Author
Owner

right, I've changed it.

right, I've changed it.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 22:00:17 +00:00
@ -0,0 +257,4 @@
}
}
.how-it-works-modal {

This style is in the wrong component

This style is in the wrong component
Author
Owner

Moved it to the helper.

Moved it to the helper.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-06 22:03:08 +00:00
@ -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?

Why using here `toISOString()` and not just return the object?
Author
Owner

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?

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

Yes that makes sense
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
konrad added 3 commits 2022-02-06 22:05:57 +00:00
konrad added 1 commit 2022-02-06 22:08:38 +00:00
continuous-integration/drone/pr Build is failing Details
7cd89b7bf1
chore: rename el
konrad added 1 commit 2022-02-06 22:10:40 +00:00
continuous-integration/drone/pr Build is failing Details
4ac7d6b9df
fix: don't reset flatpickr date
dpschen reviewed 2022-02-07 10:50:27 +00:00
@ -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?

Remove rel attribute here and below. Show we also add `target="_blank"` as default for the BaseButton is a link case?
Author
Owner

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.

> 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.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-07 10:52:26 +00:00
@ -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.

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

Changed!

Changed!
konrad marked this conversation as resolved
dpschen reviewed 2022-02-07 11:18:50 +00:00
@ -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)"

These abstractions are unnecessary and just increase complexity, call `setDateFilter` directly, like: `@dateChanged="(values) => setDateFilter('due_date', values)"`
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-07 11:24:15 +00:00
@ -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?

Wouldn't it be better to make showAll a computed that gets autoset when dateFrom and dateTo doesn't contain a value?
Author
Owner

Excellent idea. Changed it!

Excellent idea. Changed it!
konrad marked this conversation as resolved
dpschen reviewed 2022-02-07 11:26:12 +00:00
@ -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.

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.

Add `?.[0]` so we return the key, which is what we actually want to use and adjust variable name.
Author
Owner

Unsure: I think eslint doesn't complain here if you use underscore (_) to spread the unused variable.

Nope, does not seem to have an effect.

> Unsure: I think eslint doesn't complain here if you use underscore (_) to spread the unused variable. Nope, does not seem to have an effect.
Author
Owner

Add ?.[0] so we return the key, which is what we actually want to use and adjust variable name.

Done.

> Add ?.[0] so we return the key, which is what we actually want to use and adjust variable name. Done.

Nope, does not seem to have an effect.

Interesting!

> Nope, does not seem to have an effect. Interesting!
konrad marked this conversation as resolved
dpschen reviewed 2022-02-07 11:30:21 +00:00
@ -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:

if (typeof predefinedRange !== 'undefined') {
    return t(`input.datepickerRange.ranges.${predefinedRangeKey}`)
} else {
    return showAll
			? t('task.show.titleCurrent')
			: t('task.show.fromuntil', {
				from: formatDate(dateFrom, 'PPP'),
				until: formatDate(dateTo, 'PPP'),
			})
}
Remove title var and return directly: ```js if (typeof predefinedRange !== 'undefined') { return t(`input.datepickerRange.ranges.${predefinedRangeKey}`) } else { return showAll ? t('task.show.titleCurrent') : t('task.show.fromuntil', { from: formatDate(dateFrom, 'PPP'), until: formatDate(dateTo, 'PPP'), }) }
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen reviewed 2022-02-07 11:34:23 +00:00
@ -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 =)

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 =)
dpschen marked this conversation as resolved
dpschen reviewed 2022-02-07 11:37:05 +00:00
@ -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 =)

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

Done.

Done.
konrad marked this conversation as resolved
dpschen added 1 commit 2022-02-09 16:53:18 +00:00
continuous-integration/drone/pr Build is failing Details
badbae0e9a
fix: mark query parameter as string
dpschen self-assigned this 2022-02-15 13:24:54 +00:00
konrad added 2 commits 2022-02-20 20:08:05 +00:00
konrad added 1 commit 2022-02-20 20:13:51 +00:00
continuous-integration/drone/pr Build was killed Details
4195953696
chore: rename i18n key for datemath help
konrad added 2 commits 2022-02-20 20:18:08 +00:00
konrad added 1 commit 2022-02-20 20:21:09 +00:00
continuous-integration/drone/pr Build was killed Details
564f669ed4
chore: return key directly
konrad added 1 commit 2022-02-20 20:22:34 +00:00
continuous-integration/drone/pr Build is failing Details
95d8cdffe4
chore: return the title directly
konrad added 1 commit 2022-02-20 20:32:06 +00:00
konrad added 1 commit 2022-02-20 20:34:49 +00:00
continuous-integration/drone/pr Build was killed Details
622f08fb1b
fix: lint
konrad added 1 commit 2022-02-20 21:03:04 +00:00
continuous-integration/drone/pr Build is failing Details
c7943ef823
fix: popup not really hidden when hidden
konrad added 1 commit 2022-02-20 21:32:51 +00:00
continuous-integration/drone/pr Build is passing Details
da162d5652
fix: modal not scrolling content when open
konrad added 1 commit 2022-02-26 12:30:13 +00:00
continuous-integration/drone/pr Build is passing Details
d5f0158b04
Merge branch 'main' into feature/date-math
# Conflicts:
#	src/views/tasks/ShowTasks.vue
konrad added 2 commits 2022-02-27 16:21:17 +00:00
Author
Owner

@dpschen What's left to do here?

@dpschen What's left to do here?
konrad added 1 commit 2022-03-27 20:41:58 +00:00
continuous-integration/drone/pr Build is passing Details
0af6d79eff
Merge branch 'main' into feature/date-math
konrad merged commit 9b09fadbd0 into main 2022-03-28 17:30:43 +00:00
konrad deleted branch feature/date-math 2022-03-28 17:30:43 +00:00
This repo is archived. You cannot comment on pull requests.
No description provided.