[pickers] Fix `DateCalendar` timezone management #12321

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

Fixes #10804

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 value 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 f36ed3a

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

shaharyar-shamshi · 2024-03-04T18:42:45Z

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

@@ -9,7 +9,9 @@ import { createPickerRenderer, adapterToUse } from 'test/utils/pickers';
 const isJSDOM = /jsdom/.test(window.navigator.userAgent);

 describe('<DateCalendar />', () => {
- const { render, clock } = createPickerRenderer({ clock: 'fake' });
+ const { render, clock } = createPickerRenderer({


seems to be unintentional change

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> ); }

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

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

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.

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

Open

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

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

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

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

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 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

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

[pickers] Fix DateCalendar timezone management #12321

Are you sure you want to change the base?

[pickers] Fix DateCalendar timezone management #12321

Conversation

LukasTy commented Mar 4, 2024

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

LukasTy Mar 5, 2024

Choose a reason for hiding this comment

shaharyar-shamshi Mar 5, 2024

Choose a reason for hiding this comment

LukasTy Mar 5, 2024

Choose a reason for hiding this comment

shaharyar-shamshi Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

shaharyar-shamshi Mar 4, 2024

Choose a reason for hiding this comment

shaharyar-shamshi Mar 4, 2024

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

flaviendelangle Apr 18, 2024

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

[pickers] Fix `DateCalendar` timezone management #12321

[pickers] Fix `DateCalendar` timezone management #12321

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