Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add vue recurrence picker #1348

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

add vue recurrence picker #1348

wants to merge 10 commits into from

Conversation

jeriox
Copy link
Contributor

@jeriox jeriox commented Sep 7, 2024

closes #195 closes #958

  • Implemented with Vue, similar to the signup block editor.
  • Regarding the upcoming improvements in Copy shift config #958, the vue code is prepared to also pick times
  • Getting the dates for the preview is no longer done via django, but directly in JS
  • We decided against implementing a vue build step (see Refactor Vue components to SFC #1351)

ToDo

  • move widget to separate template
  • implement shift copy

@jeriox
Copy link
Contributor Author

jeriox commented Sep 7, 2024

First draft implementation:
image

@jeriox jeriox force-pushed the new-recurrence branch 2 times, most recently from 4110a43 to 26d0b04 Compare September 11, 2024 20:54
@jeriox jeriox marked this pull request as ready for review September 12, 2024 17:23
event.target.submit();
}

function isRuleValid(rule) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is stricter than the RFC, but I thought it would be suitable for us (e.g. end of recurrence mandatory, days of weeks/month need to be specified instead of being derived from DTSTART)

@lukasrad02
Copy link

The layout is not mobile-optimized. And it might make sense to not use two columns on mobile.

Screenshot of copy event view on mobile. Many controls overflow their containers

@lukasrad02
Copy link

I don't know what went wrong here, but the calculated dates are incorrect (should be 2024-09-16 and 2024-09-23).

image

However, when clicking copy, the events will be copied to the right dates.

@jeriox
Copy link
Contributor Author

jeriox commented Sep 13, 2024

I don't know what went wrong here, but the calculated dates are incorrect (should be 2024-09-16 and 2024-09-23).

image

However, when clicking copy, the events will be copied to the right dates.

what timezone is your browser in? I fought a lot with that, but on my machine it was correct in the end when I was in Europe/Berlin as well.

@lukasrad02
Copy link

what timezone is your browser in? I fought a lot with that, but on my machine it was correct in the end when I was in Europe/Berlin as well.

Europe/Berlin (according to Intl.DateTimeFormat().resolvedOptions().timeZone).

Same behavior across Firefox (130.0), Firefox Developer Edition (130.0b9) and Chromium (128.0.6613.137) on the same machine as well as Firefox for Android (130.0). All browsers have timezone Europe/Berlin which is also set on the respective OS.

@lukasrad02
Copy link

I'm not familiar with the recurrence spec, but my naive expectation with a recurrence like "weekly, Mondays and Tuesdays, for 2 occurrences" would be that there are 4 copies in total (two times Monday and Tuesday), but there are only two total copies (one time Monday and Tuesday).

The behavior is consistent across the preview in the frontend and the actual copies created in the backend, so I think it is correct behavior, but it might be worth explaining, for example with an ℹ️-icon after "occurrences" that shows a tooltip explaining the interpretation of the number (overall number of copies, not number of weeks).

@lukasrad02
Copy link

Maybe related to the timezones: By chance, I've noticed that the following pattern causes an error "Invalid recurrence rule: unsupported RDATE parm: TZID=EUROPE/BERLIN" reproducibly:

Pattern: Starting at 2024-09-14, weekly mondays 2 occurrences, additional dates 2024-09-18 and 2024-09-19

Tested in Firefox Developer Edition and Chromium, but the error seems to come from the backend, as it is rendered as {{ form.errors }}. This is the recurrence string set to the server:

DTSTART;TZID=Europe/Berlin:20240914T000000RRULE:FREQ=WEEKLY;INTERVAL=1;BYDAY=MO;COUNT=2RDATE;TZID=Europe/Berlin:20240918T000000,20240919T000000

Comment on lines 3 to 7
function formatDate(date_obj, sep = "-") {
let month = date_obj.getMonth() + 1;
let day = date_obj.getDate() + 1;
return date_obj.getFullYear() + sep + (month < 10 ? "0" : "") + month + sep + (day < 10 ? "0" : "") + day
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There must not be a + 1 for the day/date. Only the month is 0-based. This should also fix #1348 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, could you pls check again?

@jeriox
Copy link
Contributor Author

jeriox commented Sep 19, 2024

Maybe related to the timezones: By chance, I've noticed that the following pattern causes an error "Invalid recurrence rule: unsupported RDATE parm: TZID=EUROPE/BERLIN" reproducibly:

good catch, forgot to mention that in the PR. this property is currently not supported in the library, I opened a PR to fix this

@jeriox jeriox requested review from felixrindt and removed request for felixrindt October 2, 2024 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy shift config Improve date picking for event copy
3 participants