[scheduler] Timezone issue 2: use render timezone in nowUpdatedEveryMinute and date generating selectors
#20411
Conversation
…one-prop-and-state-to-scheduler
…one-prop-and-state-to-scheduler
…one-prop-and-state-to-scheduler
…imezone-issue-1-add-timezone-prop-and-state-to-scheduler
…one-prop-and-state-to-scheduler
…state-to-scheduler' of github.com:rita-codes/mui-x into 20389-scheduler-timezone-issue-1-add-timezone-prop-and-state-to-scheduler
rita-codes
added
type: new feature
…r-timezone-in-nowupdatedeveryminute-and-date-generating-selectors
|
Deploy preview: https://deploy-preview-20411--material-ui-x.netlify.app/ Bundle size report
|
|
|
||
| const expected = adapter.startOfDay(adapter.now(timezone)); | ||
|
|
||
| expect(store.state.visibleDate).toEqualDateTime(expected); |
There was a problem hiding this comment.
Since we're talking about "visibleDate" I think it makes sense to pass the rendered timezone to the callback here
There was a problem hiding this comment.
To which callback? siblingVisibleDateGetter?
There was a problem hiding this comment.
sorry, the comment should be a couple of lines below, I meant the value for onVisibleDateChange
There was a problem hiding this comment.
Could you give a code snippet with your proposal?
I'm not sure to understand.
There was a problem hiding this comment.
Sorry I didn't explain the "problem" properly 😂
We were talking about how we should handle dates "inside" and "outside" the component. As a general rule, we are treating all dates "inside" our app with the render timezone, but when we need to send that data outside, the data is going back to the user again (updates, onEventChange, etc) all the dates should be in the original timezone. That makes sense but there is an exception here, and that's onVisibleDateChange.
Even if we're returning data to the user, the information is "what's the visible date" for the final user, for the user that is using the app, and that user is seeing a date in a specific timezone, so I think we should send the date with the render timezone in onVisibleDateChange.
does this make sense to you too? 🤔
There was a problem hiding this comment.
People can still provide visibleDate in another timezone, so we need to make sure it's properly converted (in the selector I'd say, better keep the state equal to the controlled prop whenever possible. And always use the selector is a good practice).
For the onVisibleDateChange, we can keep the UI timezone. To be honest I don't know what's better. I agree with your argument, but at the same time it's not super satisfying to pass a value in another TZ than the one we were given for the previous value.
There was a problem hiding this comment.
I think we're fully aligned then.
For controlled mode, it makes sense to keep visibleDate exactly as provided, without applying any timezone logic to it. The component should treat the user-provided value as the source of truth, and any timezone normalization should only happen in the selector purely for rendering purposes. That way the value isn't modified, and the component remains properly controlled.
About onVisibleDateChange, I think returning the date in the render timezone is the most consistent with what the end-user actually sees, but I agree it's not ideal that the input and output timezones differ. I'll take a look at how other libraries handle this and bring it to the weekly meeting so we can decide together 👀
There was a problem hiding this comment.
I documented my research here, including how FullCalendar and Bryntum handle it and why the render-timezone value is the most consistent choice for onVisibleDateChange (for "UI callbacks" in general)
So I think we should follow that path: keep the controlled visibleDate untouched, do any normalization only in the selector, emit the render-timezone value for UI callbacks (onVisibleDateChange) since that’s what the final user actually sees, and as for saving/updating convert back to the original timezone (like Bryntum does).
rita-codes
changed the title
nowUpdatedEveryMinute and date generating selectors
…r-timezone-in-nowupdatedeveryminute-and-date-generating-selectors
…imezone-issue-2-use-render-timezone-in-nowupdatedeveryminute-and-date-generating-selectors
…r-timezone-in-nowupdatedeveryminute-and-date-generating-selectors
…r-timezone-in-nowupdatedeveryminute-and-date-generating-selectors
…r-timezone-in-nowupdatedeveryminute-and-date-generating-selectors
3 participants
Issue #20390
In this PR:
nowUpdatedEveryMinuteand any selector that generates “now”, “today”, or visible ranges.