[fields] Improve the check for year in doesSectionFormatHaveLeadingZeros
#17051
Conversation
flaviendelangle
added
bug 🐛
|
Thanks for adding a type label to the PR! 👍 |
flaviendelangle
changed the title
doesSectionFormatHaveLeadingZeros
flaviendelangle
added
needs cherry-pick
|
Deploy preview: https://deploy-preview-17051--material-ui-x.netlify.app/ |
| @@ -195,8 +195,7 @@ function adjustSectionValue<TValue extends PickerValidValue>( | |||
| const step = | |||
| section.type === 'minutes' && stepsAttributes?.minutesStep ? stepsAttributes.minutesStep : 1; | |||
|
|
|||
| const currentSectionValue = parseInt(removeLocalizedDigits(section.value, localizedDigits), 10); | |||
| let newSectionValueNumber = currentSectionValue + delta * step; | |||
| let newSectionValueNumber: number; | |||
There was a problem hiding this comment.
Not related, it was just weird to generate NaN on newSectionValueNumber and then not use it, I lost some time debugging because of it
There was a problem hiding this comment.
Nice and quick fix. 👍 🎉
This looks like an issue even on v6... 🤷
Not sure how it could have started happening if we didn't update anything there, but I added v6.x label to create a cherry-pick to that branch, because technically it could be seen as a "regression" that might be worth fixing. 🙈
| @@ -28,7 +28,7 @@ type Constructor = (...args: Parameters<typeof defaultDayjs>) => Dayjs; | |||
|
|
|||
| const formatTokenMap: FieldFormatTokenMap = { | |||
| // Year | |||
| YY: 'year', | |||
| YY: { sectionType: 'year', contentType: 'digit', maxLength: 2 }, | |||
There was a problem hiding this comment.
Could be related, but I noticed on vitest an error that could only be fixed by adding
yy: { sectionType: 'year', contentType: 'digit', maxLength: 2 }, to AdapterDateFnsBase.
I don't understand why this is the case or why the tests were failing only in vitest, so if you create a fix let me know, otherwise I'll isolate the change into a PR later.
There was a problem hiding this comment.
console.log(dayjs().set('year', 2009).format('YY')); // 09
console.log(dayjs().set('year', 9).format('YY')); // 9
I don't want to leave on this planet anymore 😆
There was a problem hiding this comment.
I'll keep this for a follow up
But yy: { sectionType: 'year', contentType: 'digit', maxLength: 2 }, causes a bug as well... you can't enter a year below 9 because dayjs can't parse 9 for the token YY 🤦
There was a problem hiding this comment.
For me it's a bug on dayjs side because they clearly state it should have to digits, so console.log(dayjs().set('year', 9).format('YY')) should return 09
There was a problem hiding this comment.
https://github.com/iamkun/dayjs/blob/45f88334d08f8278e51b8189671ddea4720c754e/src/index.js#L287
I think the fix is super simple here
There was a problem hiding this comment.
My comment was regarding the DateFns adapter, not dayjs
There was a problem hiding this comment.
There was a problem hiding this comment.
Oh sorry, in that case if you can extract it 🙏 , I'll have a look at it
There was a problem hiding this comment.
It seems it was fixed after merging master in 🤔
flaviendelangle
force-pushed
the
jalali-yyyy
branch
from
b29774d to
542cfbc
Compare
|
Cherry-pick PRs will be created targeting branches: v6.x, v7.x |
Fixes #17047
I can't reproduce with date-fns-jalali v3, but I think this test is more robust anyway.