[pickers] Fix `AdapterMomentJalaali` regression #13144

LukasTy · 2024-05-15T15:49:02Z

Add a docs example with moment-jalaali.

The isSameYear and especially the isSameMonth methods seemed incorrect.
Replaced with this approach:
https://github.com/dmtrKovalenko/date-io/blob/634ae738fa70545ad5fa954861a5e5498b4affc6/packages/jalaali/src/jalaali-utils.ts#L229-L235

The culprit of the regression is: https://github.com/mui/mui-x/pull/8789/files#diff-de14e1bc65d9c2f3084b3e07420b775d6bdcd1fb6b2538dc02745ab862b4a998R185-R189

LukasTy · 2024-05-15T15:49:34Z

test/e2e/index.test.ts

@@ -766,6 +766,17 @@ async function initializeEnvironment(
        //   expect(await status.isVisible()).to.equal(true);
        //   expect(await status.textContent()).to.equal('Submitted: 04/17/2022');
        // });
+
+        it('should correctly select a day in a calendar with "AdapterMomentJalaali"', async () => {


I wasn't able to make a unit test work in a reasonable amount of time. 🙈

LukasTy · 2024-05-15T15:51:42Z

packages/x-date-pickers/src/AdapterMomentJalaali/AdapterMomentJalaali.ts

-    // `isSame` seems to mutate the date on `moment-jalaali`
-    // @ts-ignore
-    return value.clone().isSame(comparing, 'jMonth');
+    return value.jYear() === comparing.jYear() && value.jMonth() === comparing.jMonth();


This is the main problem that was causing the behavior, because it is called here:

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

Lines 71 to 74 in 6c4308f

const needMonthSwitch =

action.focusedDay != null &&

!disableSwitchToMonthOnDayFocus &&

!utils.isSameMonth(state.currentMonth, action.focusedDay);

And it caused the focus to be changed to the first day of the current month.

mui-bot · 2024-05-15T15:52:17Z

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

Updated pages:

docs/data/date-pickers/calendar-systems/calendar-systems.md

Generated by 🚫 dangerJS against 3102b0a

flaviendelangle

With those methods I would be very careful with timezone
Could you check if we have tests on those adapter methods for edge cases where you are in the same year when converted to the same timezone but different years in each date's original timezone?

LukasTy · 2024-05-15T16:42:41Z

With those methods I would be very careful with timezone
Could you check if we have tests on those adapter methods for edge cases where you are in the same year when converted to the same timezone but different years in each date's original timezone?

@flaviendelangle This particular adapter is not timezone compatible. 🤔

mui-x/packages/x-date-pickers/src/AdapterMomentJalaali/AdapterMomentJalaali.ts

Line 126 in 6c4308f

public isTimezoneCompatible = false;

flaviendelangle · 2024-05-15T16:48:05Z

Right...
I'm slightly uncomfortable to add a behavior that is not timezone compatible (even if it's unlikely that we will ever add timezone support on it, I don't think we could use moment-timezone and momentj-lalaali together) and to diverge from the AdapterMoment logic (which uses value.isSame)

But that's probably not important indeed 😆

LukasTy · 2024-05-15T16:51:08Z

docs/data/date-pickers/calendar-systems/AdapterMomentJalali.js

+        <LocalizationProvider dateAdapter={AdapterMomentJalaali} adapterLocale="fa">
+          <DateTimePicker
+            label="AdapterMomentJalaali"
+            defaultValue={moment('2022-02-01T12:00:00')}


date-fns-jalaali defaults to 12hr clock, whereas moment-jalaali to 24hr clock.
I set both to mid-day to at least have them show 12:00.
Do you have better suggestions?
Maybe force ampm to false or true on both? 🤔

Not sure if adding an exception to our AMPM management to fix this inconsistency is worth it
I think your test is nice and we can keep the behavior as it is

LukasTy · 2024-05-15T17:19:55Z

The global moment locale is causing a headache in docs...
Calling moment.loadPersian in the added demo causes other demos using moment to receive the Persian locale.

There are a couple of options:

Don't call moment.loadPersian, which would change the demo to use Farsi symbols:
Call moment.locale('en') in MomentTimezone and MomentUTC demos to avoid the unwanted locale.

~~Both methods are not the prettiest and I'm really not sure which one would be the least offensive.~~ 🤔
~~However, I'm leaning towards the 1st option.~~

Edit

Forget option 1, the global moment locale does still bleed into other demos... 😱 🤯
I've gone with 2nd option, let me know if you have better suggestions. 🙏

flaviendelangle · 2024-05-16T06:28:50Z

Call moment.locale('en') in MomentTimezone and MomentUTC demos to avoid the unwanted locale.

Does it really work if the component re-renders?

LukasTy · 2024-05-16T08:02:48Z

Does it really work if the component re-renders?

@flaviendelangle I've tried going back and forth multiple times between the demo pages (jalali and Moment UTC) and changing the demos to trigger some issue, but I couldn't do it... 🤔 🤷
Were you able to produce an issue? 🤔

I would expect problems if those demos were on the same page, but it seems "fine" in the current state. 🙈 🤷

P.S. Only after ca1e423 commit argos (test:regressions) is finally happy and no longer detects those previously seen problems. 😉

flaviendelangle · 2024-05-16T08:06:33Z

Were you able to produce an issue? 🤔

No
It's quite fragile and might not work fine if we decide to merge some doc pages, but I don't have a better solution :/

LukasTy · 2024-05-16T08:17:33Z

It's quite fragile and might not work fine if we decide to merge some doc pages, but I don't have a better solution :/

I'm quite unhappy with the solution as well, but on the other hand, if we have problems—we can reconsider having this demo or moving it somewhere else.
It could also become a live example with code preview or something like that. 🤔

LukasTy added 2 commits May 15, 2024 18:46

Add e2e test asserting proper behavior with AdapterMomentJalaali

6157651

[pickers] Fix AdapterMomentJalaali regression

cb24a44

LukasTy added regression A bug, but worse l10n localization component: pickers This is the name of the generic UI component, not the React module! labels May 15, 2024

LukasTy self-assigned this May 15, 2024

LukasTy commented May 15, 2024

View reviewed changes

LukasTy added the needs cherry-pick The PR should be cherry-picked to master after merge label May 15, 2024

LukasTy added 2 commits May 15, 2024 19:38

Fix tsc

bb0e76d

Add moment-jalaali example in docs

b520516

flaviendelangle reviewed May 15, 2024

View reviewed changes

LukasTy commented May 15, 2024

View reviewed changes

LukasTy requested review from flaviendelangle, michelengelen and a team May 15, 2024 16:51

LukasTy added 4 commits May 15, 2024 20:27

Ensure en locale in other moment demos

d21e9c6

Empty commit

8445de0

Add explicit adapter locale to avoid mixing locales

ca1e423

Trigger CI... Again

3102b0a

flaviendelangle approved these changes May 16, 2024

View reviewed changes

LukasTy merged commit 40bfc8d into mui:master May 16, 2024
17 checks passed

LukasTy deleted the fix-moment-jalaali-adapter branch May 16, 2024 09:06

LukasTy mentioned this pull request May 16, 2024

[pickers] Fix AdapterMomentJalaali regression #13150

Merged

arthurbalduini pushed a commit to arthurbalduini/mui-x that referenced this pull request May 23, 2024

[pickers] Fix AdapterMomentJalaali regression (mui#13144)

f5962b7

Rossen27 mentioned this pull request Jun 22, 2024

[Snyk] Upgrade @mui/x-tree-view from 7.3.1 to 7.6.1 Rossen27/Shopit-V1#81

Open

DungTiger pushed a commit to DungTiger/mui-x that referenced this pull request Jul 23, 2024

[pickers] Fix AdapterMomentJalaali regression (mui#13144)

7e6d471

thomasmoon pushed a commit to thomasmoon/mui-x that referenced this pull request Sep 9, 2024

[pickers] Fix AdapterMomentJalaali regression (mui#13144)

b67bd02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pickers] Fix `AdapterMomentJalaali` regression #13144

[pickers] Fix `AdapterMomentJalaali` regression #13144

LukasTy commented May 15, 2024 •

edited

Loading

LukasTy May 15, 2024

LukasTy May 15, 2024

mui-bot commented May 15, 2024 •

edited

Loading

flaviendelangle left a comment

LukasTy commented May 15, 2024

flaviendelangle commented May 15, 2024

LukasTy May 15, 2024

flaviendelangle May 15, 2024

LukasTy commented May 15, 2024 •

edited

Loading

flaviendelangle commented May 16, 2024

LukasTy commented May 16, 2024 •

edited

Loading

flaviendelangle commented May 16, 2024

LukasTy commented May 16, 2024

	const needMonthSwitch =
	action.focusedDay != null &&
	!disableSwitchToMonthOnDayFocus &&
	!utils.isSameMonth(state.currentMonth, action.focusedDay);

[pickers] Fix AdapterMomentJalaali regression #13144

[pickers] Fix AdapterMomentJalaali regression #13144

Conversation

LukasTy commented May 15, 2024 • edited Loading

LukasTy May 15, 2024

Choose a reason for hiding this comment

LukasTy May 15, 2024

Choose a reason for hiding this comment

mui-bot commented May 15, 2024 • edited Loading

flaviendelangle left a comment

Choose a reason for hiding this comment

LukasTy commented May 15, 2024

flaviendelangle commented May 15, 2024

LukasTy May 15, 2024

Choose a reason for hiding this comment

flaviendelangle May 15, 2024

Choose a reason for hiding this comment

LukasTy commented May 15, 2024 • edited Loading

Edit

flaviendelangle commented May 16, 2024

LukasTy commented May 16, 2024 • edited Loading

flaviendelangle commented May 16, 2024

LukasTy commented May 16, 2024

[pickers] Fix `AdapterMomentJalaali` regression #13144

[pickers] Fix `AdapterMomentJalaali` regression #13144

LukasTy commented May 15, 2024 •

edited

Loading

mui-bot commented May 15, 2024 •

edited

Loading

LukasTy commented May 15, 2024 •

edited

Loading

LukasTy commented May 16, 2024 •

edited

Loading