feat: Edit relative reminders #3248
dpschen
konrad
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
4 Participants
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/frontend#3248
Loading…
Reference in New Issue
No description provided.
Delete Branch "ce72/frontend:main"
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 is a full functional POC to use the new reminder API from vikunja/api#1427.
Maybe the styling can be improved, but I am not very good at that.
(The changes from the current PR require vikunja/api#1427. But vikunja/api#1427 supports both the old and the new frontend.)
Related to: #3216
Hi ce72!
Thank you for creating a PR!
I've deployed the changes of this PR on a preview environment under this URL: https://3248-main--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!
a64fcfc4c1tob09b5622aaWIP: feat: Edit relative remindersto feat: Edit relative reminders@ -0,0 +52,4 @@}function showRelativeReminder() {return !reminder.value || reminder.value?.relativeToPlease use a
computedfor bothshow...functions instead.ok
@ -0,0 +1,207 @@<template><div class="datepicker"><BaseButton :disabled="disabled" class="show" v-if="!!reminder?.relativeTo" @click.stop="togglePeriodPopup">{{ formatDuration(reminder.relativePeriod) }} <span v-html="formatBeforeAfter(reminder.relativePeriod)"></span>What about inlining
formatBeforeAfterto something like{{ reminder.relativePeriod <= 0 ? '≤' : '>' }}ok
@ -0,0 +14,4 @@v-model="periodInput.duration.days"type="number"min="0"/> dPlease add a translation string for the
d. (It does not look like much, but still)Same for the placeholders.
ok
To be honest I was confused by the
dwhen I first saw the interface.Why don't we write it out?
ok
@ -0,0 +196,4 @@<style lang="scss" scoped>.input {max-width: 70px;width: 70px;Please use `rem
ok
@ -57,4 +35,2 @@return true},},disabled: {Why remove the type?
The type has changed from
Date | Stringto a complex type (Array as PropType<ITaskReminder[]>). I don't see why a validation is required. What do you suggest?@ce72: Sry I don't understand your answer here :)
What has the validation of the modelValue to do with the type of disabled?
Can you please make a concrete suggestion what you expect here? I can find no other place in the code where you have a validation on a complex props type. And am not enough typescript native to understand what you want to have added here.
I think I misunderstood @konrads first question, because the lines changed. I assumed he is refering to the removed type from disabled and not the removed validation of the modelValue.
Then my question goes to @konrad
Okay that actually makes sense.
@ -0,0 +3,4 @@/*** Convert time period given as seconds to days, hour, minutes, seconds*/export function secondsToPeriod(seconds: number) {Can you add a type hint for the return value?
ok
Couldn't we instead define the return via
as const?Also we don' really need the declaration of the variables but could inline those definitions in the returned object directly
OK concerning the inlining. Can you please make a suggestion on how to replace the function using
as const? My typescript might be not sufficient here.I wonder if it would make sense to add a new component for absolute reminder dates?
The code uses Datepicker for absolute reminders. There might be advantages to write a wrapper around that, but given my web programming skills, I don't see how (and why) that should be done now.
@ -74,3 +76,3 @@repeatFromCurrentDate = falserepeatMode: IRepeatMode = TASK_REPEAT_MODES.REPEAT_MODE_DEFAULTreminderDates: Date[] = []reminderDates = nullIf
reminderDatesisnullwhy do we still define it?We want to make sure to not return reminderDates to the server. The api currently supports both reminderDates (old API) and reminder (new API). If the frontend still sends the old reminderDates array back to the server, it assumes the old logic and ignores the new reminders. The whole thing will be removed, if the reminderDates will be entirely removed from the server.
Can you add this as comment to this line?
ok
9923794e74to7a7ff5a28e@ -0,0 +1,64 @@<template><div><ReminderPeriod v-if="showRelativeReminder" v-model="reminder" :disabled="disabled"Nitpicking, but please format this like this:
That way it's more readable.
Fixed in my changes, see #3248 (comment)
@ -0,0 +95,4 @@const periodInput = reactive({duration: {days: 0, hours: 0, minutes: 0, seconds: 0},relativeTo: REMINDER_PERIOD_RELATIVE_TO_TYPES.DUEDATE,sign: -1,What does
signmean in this context?What's the advantage of using an object here instead of individual
refs?@ -789,0 +790,4 @@"hoursShort": "HH","minutesShort": "MM","daysShort": "d","days": "days"Is it intended this is not
Days? (Capital d)Could we use
Intl.DisplayNamesto get the names instead?See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DisplayNames/of
Will try to use that in my changes.
@ -77,0 +78,4 @@/* Make sure to not return reminderDates to the server.The server currently supports both reminderDates (old API) and reminder (new API) and assumes the old logicif it still receives reminderDates.This line and reminderDates attributes will be removed after https://kolaente.dev/vikunja/api/pulls/1448 was merged.But then you'll need another PR after this one and the api change to remove the old reminder_dates were merged? Wouldn't it be easier to remove all traces of the old reminder dates in this PR already and then only merge the api one to clean up the leftovers in the api?
The server still sends old reminderDates (in order to be non-breaking). These must not be returned by the frontend, or that would signal the server that the frontend prefers the old API. The reminderDates are set to null, or they will still be included (two places).
Maybe that's not the only migration strategy to avoid breaking changes, but I think now we should more or less stick to it. I will take care to remove reminderDates from the frontend in a final PR on this.
Can you add a cypress test for the new reminders?
EDIT (Dominik)
Please wait with this. See: #3248 (comment)
I wonder if we could move this and the date picker into one popup:
This is how TickTick does it:
We don't nessecarily need to do this here in this PR, just something to think about.
@ce72
I'm refactoring this pr a bit, since I have the feeling that noting what I would change would take much longer than just doing it. Hopefully you're fine with that considering you mentioned
but I am not very good at that.You can see what I did here until now: https://kolaente.dev/vikunja/frontend/compare/main...dpschen/frontend:edit-relative-reminders
It's currently in a broken in-between state.
When I'm finished with this I still do not want to immediately merge this, but instead take a little step back and think a bit in the direction that @konrad mentioned in his comment and change the implementation in that direction. Shouldn't be toooo hard after my changes.
The result doesn't have to be perfect (since I tend to strive in that direction I want to explicitly point this out), but the user should at least understand what the interface is for / doing. When I tried this the first time I must honestly say that I had no clue what I can select in this interface and what it is doing. That said I'm really happy that we are able to build on your fundamental work. Without your start this feature probably wouldn't have happened soon. 💯 💪
Sidenote:
Please don't merge in your branch if it's not really necessary. Merge commits make rebasing much harder, which is what we prefer when finally merging the pr.
Please wait before doing this. I added the component to histoire (see
pnpm run story:dev) which makes building components in isolation / manual testing much easier.Great. I'm very fine with this.
Reopened
@ce72
Sry didn't want to edit your comment – reversed that.
I thought I made a mistake creating my comment, but your answer simply consisted of a quote of my comment instead.
Relative reminders are now part of api:unstable and are coming in via CalDav sync and can be edited with beta version of vja. But at the moment the frontend overwrites relative reminders to absolute reminder dates. I think this should be fixed soon, by merging this PR or at least part of it (see also vikunja/api#1448 (comment)).
Can we maybe just split the make-it-work part from the make-it-nice part? I think that being more agile here and having more and smaller (focused) PRs, that actually get merged, would be much more encouraging to engage further.
@dpschen please let me know if I can help!
I've now implemented a datepicker very similar to the one from TickTick, while incorporating the changes @dpschen already made.
Here's what it looks like:
There's still little rough edges, but I think it's a really good improvement already.
Thanks again @ce72 and @dpschen!
I have been testing this now for quite some time and am really happy with it.
(Just three things:
)
Thanks for getting this change home. I really like Vikunja even more now!
@ce72 Thanks for your feedback!
I've improved the things you mentioned plus something I noticed myself, see
1a792e0667,928b338cf2,32e5f9f757and5694b39489Just another 2 cents from me:
Added the two extra presets, but didn't manage to find the cause for the error message. It does not happen in Dev builds and the built files make it hard to debug. I guess that's another reason why we finally need source maps in sentry^^