WIP: feat: add default reminder for tasks with a due date #2360
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#2360
Loading…
Reference in New Issue
No description provided.
Delete Branch "feature/add-default-reminder"
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 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)
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!
@ -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.
Done.
@ -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?
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?
It might.
@ -596,6 +588,22 @@
}
}
},
"time": {
Use vue-i18n pluralisation feature.
Or didn't you use that to be more because of crowdin?
I actually just forgot about that feature :) will change it.
Done.
@ -14,2 +13,4 @@
weekStart: IUserSettings['weekStart'] = 0
timezone = ''
defaultReminder = false
defaultReminderAmount = 0
se here
getDefaultReminderAmount
Done.
@ -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.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
I've combined them.
@ -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).
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.
9a52c529c8
toe65c286730
@ -0,0 +1,90 @@
const DEFAULT_REMINDER_KEY = 'defaultReminder'
export const AMOUNTS_IN_SECONDS: {
Reuse type definition. E.g.
Done.
@ -0,0 +10,4 @@
hours: 60 * 60,
days: 60 * 60 * 24,
months: 60 * 60 * 24 * 30,
}
Add
as const
Done.
@ -0,0 +23,4 @@
type?: 'minutes' | 'hours' | 'days' | 'months',
}
function calculateDefaultReminderSeconds(type: string, amount: number): number {
type
would better be askeyof typeof AMOUNT_IN_SECONDS
.Maybe we should save the
type
as new type, e.g.I've changed it to use
SavedReminderSettings['type']
instead.@ -0,0 +36,4 @@
}
export function getDefaultReminderAmount(): number | null {
const settings = getDefaultReminderSettings()
picky: don't create one-use-variable
But here I'm using it twice?
Beeing too picky is also stupid… you are right!
@ -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.
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.
@ -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?
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.feat: add default reminder for tasks with a due dateto WIP: feat: add default reminder for tasks with a due date