WIP: feat: add default reminder for tasks with a due date #2360

Draft
konrad wants to merge 17 commits from feature/add-default-reminder into main
Owner

This PR adds a user setting to automatically set a reminder for a task when a due date is set for it and it does not have at least one reminder yet.

Actually setting the reminder is implemented in the task service to make sure this works everywhere when creating a new task (like quick add magic etc).

EDIT (dpschen):
We should wait until we can save this in the user info so that this feature is reliable from the start. #2360 (comment)

This PR adds a user setting to automatically set a reminder for a task when a due date is set for it and it does not have at least one reminder yet. Actually setting the reminder is implemented in the task service to make sure this works everywhere when creating a new task (like quick add magic etc). **EDIT (dpschen):** We should wait until we can save this in the user info so that this feature is reliable from the start. https://kolaente.dev/vikunja/frontend/pulls/2360#issuecomment-36303
konrad requested review from dpschen 2022-09-15 11:35:26 +00:00
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://2360-feature-add-default-reminder--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://2360-feature-add-default-reminder--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.
dpschen self-assigned this 2022-09-29 09:02:50 +00:00
dpschen reviewed 2022-09-29 11:39:09 +00:00
@ -0,0 +13,4 @@
function calculateDefaultReminderSeconds(type: string, amount: number): number {
switch (type) {
case 'minutes':

We should save the factors in array in a mapping constant. This way the constant can be reused for the test.

We should save the factors in array in a mapping constant. This way the constant can be reused for the test.
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
@ -0,0 +51,4 @@
return JSON.parse(s)
}
export function parseSavedReminderAmount(amountSeconds: number): SavedReminderSettings {

Why don't we save the type as well? Wouldn't that make thinks simpler?

Why don't we save the type as well? Wouldn't that make thinks simpler?
Author
Owner

I guess I just thought about having the seconds ready to do the calculation, because that's what we need at the end.

I guess I just thought about having the seconds ready to do the calculation, because that's what we need at the end.

Wouldn't it be easier to keep the format until it's actually displayed?

Wouldn't it be easier to keep the format until it's actually displayed?
Author
Owner

It might.

It might.
@ -596,6 +588,22 @@
}
}
},
"time": {

Use vue-i18n pluralisation feature.
Or didn't you use that to be more because of crowdin?

Use [vue-i18n pluralisation](https://vue-i18n.intlify.dev/guide/essentials/pluralization.html) feature. Or didn't you use that to be more because of crowdin?
Author
Owner

I actually just forgot about that feature :) will change it.

I actually just forgot about that feature :) will change it.
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
@ -14,2 +13,4 @@
weekStart: IUserSettings['weekStart'] = 0
timezone = ''
defaultReminder = false
defaultReminderAmount = 0

se here getDefaultReminderAmount

se here `getDefaultReminderAmount`
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
@ -71,0 +77,4 @@
model.reminderDates.push(formatISO(reminderDate))
}
}

Below seems to be the same calculation as in the new calculateDefaultReminderSeconds function. We should probably combine these.

Below seems to be the same calculation as in the new `calculateDefaultReminderSeconds` function. We should probably combine these.
Author
Owner

Looks very similar, yes.

Looks very similar, yes.
@ -56,6 +56,10 @@
}
}
.field.has-addons .select select {

Combine with selector below.

Maybe better: use class for specific case in General

Combine with selector below. Maybe better: use class for specific case in General
Author
Owner

I've combined them.

I've combined them.
konrad marked this conversation as resolved
@ -21,0 +44,4 @@
<div class="control select">
<select v-model="defaultReminderAmountType">
<option value="minutes">
{{ $t('time.minute' + (defaultReminderAmount === 1 ? '' : 's')) }}

In case we don't use vue-i18n pluralisation feature we shouldn't use local messages that can't be precompiled (see: https://vue-i18n.intlify.dev/guide/advanced/optimization.html#optimization).

{{ defaultReminderAmount === 1 ? $t('time.minute') : $t('time.minutes') }}
In case we don't use vue-i18n pluralisation feature we shouldn't use local messages that can't be precompiled (see: https://vue-i18n.intlify.dev/guide/advanced/optimization.html#optimization). ```ts {{ defaultReminderAmount === 1 ? $t('time.minute') : $t('time.minutes') }} ```
konrad marked this conversation as resolved
Member

I think we shouldn't implement this with localstorage, because it's really unreliable. Imagine relying on that a default reminder is created, since you defined that setting, but forgetting that you logged out in between.

We should wait until we can save this in the user info so that this feature is reliable from the start.

I think we shouldn't implement this with localstorage, because it's really unreliable. Imagine relying on that a default reminder is created, since you defined that setting, but forgetting that you logged out in between. We should wait until we can save this in the user info so that this feature is reliable from the start.
konrad force-pushed feature/add-default-reminder from 9a52c529c8 to e65c286730 2022-09-29 16:17:09 +00:00 Compare
konrad added 2 commits 2022-09-29 16:23:13 +00:00
konrad added 1 commit 2022-09-29 16:30:50 +00:00
continuous-integration/drone/pr Build is passing Details
2aee048f61
fix: use vue-i18n pluralization
konrad added 1 commit 2022-09-29 16:32:01 +00:00
konrad added 1 commit 2022-09-29 16:32:55 +00:00
continuous-integration/drone/pr Build is passing Details
a341dbd5d2
fix: combine related css classes
dpschen reviewed 2022-09-29 18:28:10 +00:00
@ -0,0 +1,90 @@
const DEFAULT_REMINDER_KEY = 'defaultReminder'
export const AMOUNTS_IN_SECONDS: {

Reuse type definition. E.g.

{ [type in SavedReminderSettings['type']]: number }
Reuse type definition. E.g. ```ts { [type in SavedReminderSettings['type']]: number } ```
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen reviewed 2022-09-29 18:29:06 +00:00
@ -0,0 +10,4 @@
hours: 60 * 60,
days: 60 * 60 * 24,
months: 60 * 60 * 24 * 30,
}

Add as const

Add `as const`
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen reviewed 2022-09-29 18:32:21 +00:00
@ -0,0 +23,4 @@
type?: 'minutes' | 'hours' | 'days' | 'months',
}
function calculateDefaultReminderSeconds(type: string, amount: number): number {

type would better be as keyof typeof AMOUNT_IN_SECONDS.

Maybe we should save the type as new type, e.g.

type IReminderDurationType = 'minutes' | 'hours' | 'days' | 'months'
`type` would better be as `keyof typeof AMOUNT_IN_SECONDS`. Maybe we should save the `type` as new type, e.g. ```ts type IReminderDurationType = 'minutes' | 'hours' | 'days' | 'months' ```
Author
Owner

I've changed it to use SavedReminderSettings['type'] instead.

I've changed it to use `SavedReminderSettings['type']` instead.
konrad marked this conversation as resolved
dpschen reviewed 2022-09-29 18:33:11 +00:00
@ -0,0 +36,4 @@
}
export function getDefaultReminderAmount(): number | null {
const settings = getDefaultReminderSettings()

picky: don't create one-use-variable

picky: don't create one-use-variable
Author
Owner

But here I'm using it twice?

But here I'm using it twice?

Beeing too picky is also stupid… you are right!

Beeing too picky is also stupid… you are right!
dpschen marked this conversation as resolved
dpschen reviewed 2022-09-29 18:38:49 +00:00
@ -699,1 +699,4 @@
this.task = await this.$store.dispatch('tasks/update', task)
// Activate new fields which may have been set from the api
this.$nextTick(() => this.setActiveFields())

I don't understand why the nextTick is necessary here.

I don't understand why the nextTick is necessary here.
Author
Owner

The comment is wrong but the tick is required to make sure the maybe newly set reminder would be shown in the task detail view as well. I'll clarify the comment.

The comment is wrong but the tick is required to make sure the maybe newly set reminder would be shown in the task detail view as well. I'll clarify the comment.
dpschen reviewed 2022-09-29 18:40:24 +00:00
@ -274,0 +322,4 @@
const defaultReminderEnabled = ref<boolean>(reminderSettings?.enabled || false)
// TODO: re-populate amount and type
const defaultReminderAmount = ref(reminderSettings?.amount || 1)

Shouldn't the fallback be the same as the default in the model?

Shouldn't the fallback be the same as the default in the model?
Author
Owner

Actually not, because this is what gets shown in the input when setting this for the first time. I felt like putting a 0 here would feel weired.

Actually not, because this is what gets shown in the input when setting this for the first time. I felt like putting a `0` here would feel weired.
konrad marked this conversation as resolved
konrad added 2 commits 2022-09-30 11:28:07 +00:00
konrad added 2 commits 2022-09-30 11:32:48 +00:00
dpschen changed title from feat: add default reminder for tasks with a due date to WIP: feat: add default reminder for tasks with a due date 2023-01-17 13:28:49 +00:00
This repo is archived. You cannot comment on pull requests.
No description provided.