[pickers] Fix `DateCalendar` timezone management #12321

LukasTy · 2024-03-04T14:19:50Z

Fixes #10804.
Fixes #14453.

Closes #12256

The referenceDate and calendarState.currentMonth were not updating when the timezone prop changes.

Changed to:

update the internal referenceDate value when the provided referenceDate or timezone prop change
update internal calendarState.currentMonth timezone when referenceDate changes

mui-bot · 2024-03-04T14:24:56Z

Deploy preview: https://deploy-preview-12321--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against d4cadab

shaharyar-shamshi

Added some tentative review. will check further by running it.

shaharyar-shamshi · 2024-03-04T18:41:39Z

packages/x-date-pickers-pro/src/DateRangeCalendar/timezone.DateRangeCalendar.test.tsx

+        screen.getAllByRole('gridcell', {
+          name: (_, element) => element.nodeName === 'BUTTON',
+        }).length,
+      ).to.equal(30);


I think the correct assertion should be number of grid cell of type button before and after the timezone change should be same. (I assume for month of february and month having 31 days it might fail)

and more accurate way of doing this will be before the the timezone change create the map of index of date in gridcell and compare it after the timezone change

Thanks for noting. 🙏
I've seen your tests and do agree that they are more "complete", but IMHO, I don't see the benefit of having a more complex-to-read test with marginally stricter assertions. The main thing that we need to assert here is that the same amount of days have been rendered after a timezone switch. 🤔
I do value the benefit of simple tests, which you can glance over and understand in seconds. 🙈
What do you think?

I totally agree simple is better. But there might be chance in month of july and August also december 2024 and January 2025 when consecutive months have equal number of dates then this might not cover those edge cases.

But there might be chance in month of july and August also december 2024 and January 2025 when consecutive months have equal number of dates then this might not cover those edge cases.

Why do you think like that? 🤔
The test is set to use 1 calendar for simplicity and ("current") time is mocked to 2022-06-15.

Maybe you are talking about a separate issue (#10584) with AdapterDayjs and DST?
That's a separate topic that I'm looking into. 😉

No I am not talking about DST. although I missed the date mocking part. I assume it would render the calendar of current.

With date mocking your test will work.

Sorry for missing date mocking part.

#12321 (comment)

I think this is bug

packages/x-date-pickers/src/DateCalendar/tests/DateCalendar.test.tsx

shaharyar-shamshi · 2024-03-04T18:43:19Z

packages/x-date-pickers/src/DateCalendar/tests/timezone.DateCalendar.test.tsx

+      }
+      render(<DateCalendarWithControlledTimezone />);
+
+      expect(


same as above you can check my PR I have done in same way

shaharyar-shamshi · 2024-03-04T21:50:04Z

I think there is some bug in BasicDateRange selecting any range (let's assume 12 March to April 21) having America/New_York as the timezone now change the timezone to UTC

there is no change in the range of the calendar. also when switching while you will notice some flicker.

Same is happening with the DateCalendar

@LukasTy

github-actions · 2024-03-11T10:44:05Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

…-timezone

jesse-garrison-panther · 2024-03-28T22:56:06Z

Hello all, just checking in on this and trying to understand where things are at timeline wise. It looks like this PR fixes an issue I ran into a while ago, so I'm looking forward to this being merged. Is this PR unable to be merged because there is a dependency on a fix for this other bug?

Thanks for any info you can provide!

LukasTy · 2024-03-29T05:37:19Z

Hello all, just checking in on this and trying to understand where things are at timeline wise. It looks like this PR fixes an issue I ran into a while ago, so I'm looking forward to this being merged. Is this PR unable to be merged because there is a dependency on a fix for this other bug?

Thanks for any info you can provide!

Hey @jesse-garrison-panther, this PR fixes your mentioned problems.
It got delayed because I was looking at fixing a different problem, but it would probably make sense to limit the scope of this PR to only fixing the particular problem with the Calendar months calculation and leave the AdapterDayjs issues with DST separate. 🤔

jesse-garrison-panther · 2024-03-29T18:42:56Z

Hey @jesse-garrison-panther, this PR fixes your mentioned problems. It got delayed because I was looking at fixing a different problem, but it would probably make sense to limit the scope of this PR to only fixing the particular problem with the Calendar months calculation and leave the AdapterDayjs issues with DST separate. 🤔

@LukasTy Thanks for the clarification! It would be great if this fix could go out separately from the DST issue you mentioned, although I don't have enough context around that other issue and how it might affect this fix. If they can both be fixed in a similar timeline, that's fine too. I just know personally I've been looking forward to using a new DatePicker component our team built, but the bug I originally filed is preventing us from using it until it's fixed.

Thanks for all the great work you and the team is providing with MUI though!

flaviendelangle · 2024-04-18T11:35:43Z

packages/x-date-pickers/src/DateCalendar/useCalendarState.tsx

+  React.useEffect(() => {
+    dispatch({
+      type: 'changeMonth',
+      newMonth: utils.startOfMonth(referenceDate),


Are we sure this change is wanted?
From what I can understand it will reset the displayed month to referenceDate, so if the user had moved to another month and then the timezone changes it might be weird.

Tbh this is really an edge case and I could understand that we don't want to optimize for it

More problematic, this will break if the component re-renders and referenceDate is passed directly is not memoized (which we support for now and we should keep supporting IMHO):

import * as React from 'react'; import dayjs from 'dayjs' import Stack from "@mui/material/Stack"; import Switch from "@mui/material/Switch"; import { DemoContainer } from '@mui/x-date-pickers/internals/demo'; import { AdapterDayjs } from '@mui/x-date-pickers/AdapterDayjs'; import { LocalizationProvider } from '@mui/x-date-pickers/LocalizationProvider'; import { DateCalendar } from '@mui/x-date-pickers/DateCalendar'; export default function BasicDateCalendar() { const [isActive, setIsActive] = React.useState(false) return ( <Stack spacing={2}> <Switch value={isActive} onChange={event => setIsActive(event.target.checked)} /> <LocalizationProvider dateAdapter={AdapterDayjs}> <DemoContainer components={['DateCalendar']}> <DateCalendar referenceDate={dayjs('2022-04-17')} /> </DemoContainer> </LocalizationProvider> </Stack> ); }

@flaviendelangle WDYT about the updated solution: f859853?
It would update only the timezone and optionally in some cases the month (to avoid changing the month when switching timezone to -the current one). I.e. America/New_York -> America/Los_Angeles

From what I understand, of the two problems I describe above, the 1st one still occurs but the 2nd does not, right?

Were you able to replicate the first mentioned problem?
It does not happen to me. 🤔
This part: https://github.com/mui/mui-x/pull/12321/files#diff-b671464bcd4d0027334e53087da5e27c98342ac1c23b1550a4556879d6f20300R64-R66 is to ensure that the month doesn't shift when timezone changes to a minus offset.

Example on master

Change the month to be July using the arrows, then click on "Change timezone".
It should move back to April.

The funny thing is, we already have this behavior on master (I would have thought otherwise), so no regression in your PR 👍

That's a bit strange behavior that might be worth investigating a bit more in-depth.
I'm a bit puzzled why the current month doesn't change when the current month is changed to May (+1). 🤔

Not a clue 😆
If you can create an issue and add it to the board so that we don't loose track of it
I don't think it's the top priority right now but I agree it would be interesting to understand what is going on

…-timezone

github-actions · 2024-05-29T11:28:47Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

…-timezone

jesse-garrison-panther · 2024-07-17T17:53:25Z

Hello all, I'm just following up on this issue to see if there is a timeline of when it's expected to be merged? We've built a new date picker component that we're waiting to use, but this bug is currently a blocker. FWIW, we're also part of the Pro Plan. Thanks for any updates you can provide! 🙏

LukasTy · 2024-07-18T06:21:47Z

Hello all, I'm just following up on this issue to see if there is a timeline of when it's expected to be merged?

@jesse-garrison-panther Thank you for bringing this topic up.
We plan to finish this PR within around a month.
Is that a reasonable timeframe?

jesse-garrison-panther · 2024-07-18T18:14:08Z

We plan to finish this PR within around a month. Is that a reasonable timeframe?

@LukasTy Thanks for the reply. Yes, I think that timeframe will work.

…-timezone

flaviendelangle · 2024-10-25T09:02:27Z

packages/x-date-pickers/src/DateCalendar/useCalendarState.tsx

@@ -54,6 +55,21 @@ export const createCalendarStateReducer =
          isMonthSwitchingAnimating: !reduceAnimations,
        };

+      case 'changeMonthTimezone': {


I'm not convinced by the name of this action.
If it runs every time the referenceDate updates, could it be named changeReferenceDate?
Or if we want to keep the focus on the month, changeCurrentMonthIfTimezoneChanged?

If it runs every time the referenceDate updates, could it be named changeReferenceDate?

I don't have a strong preference for the action name.
It depends on how we look at the naming:

Should it refer to what triggered the action?

Should it refer to what the action will do?

Or if we want to keep the focus on the month, changeCurrentMonthIfTimezoneChanged?

But essentially, it does not change the currentMonth most of the time, unless it would cause the month to change because of a timezone change. 🤔

If you pass the timezone dirrectly to changeMonthTimezone instead of passing the new month, the name and its behavior would then be coherent IMHO

LukasTy added 3 commits March 4, 2024 14:03

Add DateCalendar timezone switching test

5f0fcc6

Add DateRangeCalendar timezone switching test

a2d89a1

Update currentMonth with correct timezone

c0f2e3b

LukasTy added bug 🐛 Something doesn't work component: pickers This is the name of the generic UI component, not the React module! needs cherry-pick The PR should be cherry-picked to master after merge time-zone Issues about time zone management labels Mar 4, 2024

LukasTy self-assigned this Mar 4, 2024

LukasTy changed the title ~~Fix date calendar timezone~~ [pickers] Fix DateCalendar timezone management Mar 4, 2024

LukasTy mentioned this pull request Mar 4, 2024

[pickers] fix the issue of the timezone change in Day calendar #12256

Closed

LukasTy requested review from flaviendelangle and michelengelen March 4, 2024 14:33

shaharyar-shamshi reviewed Mar 4, 2024

View reviewed changes

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 11, 2024

LukasTy changed the base branch from next to master March 25, 2024 14:55

Merge remote-tracking branch 'upstream/master' into fix-date-calendar…

f36ed3a

…-timezone

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 26, 2024

LukasTy force-pushed the fix-date-calendar-timezone branch from 9007ca4 to f36ed3a Compare April 4, 2024 12:05

flaviendelangle reviewed Apr 18, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fix-date-calendar…

b0326d8

…-timezone

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 29, 2024

Merge remote-tracking branch 'upstream/master' into fix-date-calendar…

fbaa023

…-timezone

michelengelen removed their request for review June 10, 2024 09:34

Merge remote-tracking branch 'upstream/master' into fix-date-calendar…

6c9b6d3

…-timezone

Merge remote-tracking branch 'upstream/master' into fix-date-calendar…

f78fd37

…-timezone

LukasTy added the component: DatePicker The React component. label Oct 24, 2024

Update only timezone of the currentMonth

f859853

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 25, 2024

Try using flags

2662acc

LukasTy requested a review from flaviendelangle October 25, 2024 08:33

flaviendelangle reviewed Oct 25, 2024

View reviewed changes

LukasTy added 2 commits October 25, 2024 12:33

Remove irrelevant dep

1a25a09

Retry flags

f941d02

LukasTy requested a review from arthurbalduini October 25, 2024 11:10

Revert adapters codecov project setup

22d6df9

flaviendelangle approved these changes Oct 25, 2024

View reviewed changes

LukasTy added 4 commits October 25, 2024 15:11

Code review: Flavien

f004a52

Ignore flaky line—it's fully covered in tests

694928d

Remove pointless istanbul ignore

99413ae

Test "default" timezone

d4cadab

LukasTy merged commit 0e598b2 into mui:master Oct 25, 2024
18 checks passed

LukasTy deleted the fix-date-calendar-timezone branch October 25, 2024 13:21

github-actions bot pushed a commit that referenced this pull request Oct 25, 2024

[pickers] Fix DateCalendar timezone management (#12321)

7d2c714

github-actions bot mentioned this pull request Oct 25, 2024

[pickers] Fix DateCalendar timezone management (@LukasTy) #15119

Merged

LukasTy mentioned this pull request Oct 25, 2024

[pickers] Investigate strange DateCalendar current month behavior when changing timezone #15123

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pickers] Fix `DateCalendar` timezone management #12321

[pickers] Fix `DateCalendar` timezone management #12321

LukasTy commented Mar 4, 2024 •

edited

Loading

mui-bot commented Mar 4, 2024 •

edited

Loading

shaharyar-shamshi left a comment •

edited

Loading

shaharyar-shamshi Mar 4, 2024 •

edited

Loading

LukasTy Mar 5, 2024

shaharyar-shamshi Mar 5, 2024

LukasTy Mar 5, 2024

shaharyar-shamshi Mar 5, 2024 •

edited

Loading

shaharyar-shamshi Mar 4, 2024

shaharyar-shamshi commented Mar 4, 2024 •

edited

Loading

github-actions bot commented Mar 11, 2024

jesse-garrison-panther commented Mar 28, 2024

LukasTy commented Mar 29, 2024

jesse-garrison-panther commented Mar 29, 2024

flaviendelangle Apr 18, 2024

LukasTy Oct 25, 2024

flaviendelangle Oct 25, 2024

LukasTy Oct 25, 2024

flaviendelangle Oct 25, 2024

LukasTy Oct 25, 2024

flaviendelangle Oct 25, 2024

github-actions bot commented May 29, 2024

jesse-garrison-panther commented Jul 17, 2024

LukasTy commented Jul 18, 2024

jesse-garrison-panther commented Jul 18, 2024

flaviendelangle Oct 25, 2024

LukasTy Oct 25, 2024

flaviendelangle Oct 25, 2024

[pickers] Fix DateCalendar timezone management #12321

[pickers] Fix DateCalendar timezone management #12321

Conversation

LukasTy commented Mar 4, 2024 • edited Loading

mui-bot commented Mar 4, 2024 • edited Loading

shaharyar-shamshi left a comment • edited Loading

Choose a reason for hiding this comment

shaharyar-shamshi Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaharyar-shamshi Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaharyar-shamshi commented Mar 4, 2024 • edited Loading

github-actions bot commented Mar 11, 2024

jesse-garrison-panther commented Mar 28, 2024

LukasTy commented Mar 29, 2024

jesse-garrison-panther commented Mar 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 29, 2024

jesse-garrison-panther commented Jul 17, 2024

LukasTy commented Jul 18, 2024

jesse-garrison-panther commented Jul 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[pickers] Fix `DateCalendar` timezone management #12321

[pickers] Fix `DateCalendar` timezone management #12321

LukasTy commented Mar 4, 2024 •

edited

Loading

mui-bot commented Mar 4, 2024 •

edited

Loading

shaharyar-shamshi left a comment •

edited

Loading

shaharyar-shamshi Mar 4, 2024 •

edited

Loading

shaharyar-shamshi Mar 5, 2024 •

edited

Loading

shaharyar-shamshi commented Mar 4, 2024 •

edited

Loading