Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[fields] Fix new index calculation on letter section #13310

Merged
merged 10 commits into from
Jun 6, 2024
Merged

[fields] Fix new index calculation on letter section #13310

merged 10 commits into from
Jun 6, 2024

Conversation

arthurbalduini
Copy link
Member

@arthurbalduini arthurbalduini requested a review from LukasTy May 30, 2024 12:01
@mui-bot
Copy link

mui-bot commented May 30, 2024 •
edited
Loading

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

Generated by 🚫 dangerJS against 5f9d996

@arthurbalduini arthurbalduini added the component: pickers This is the name of the generic UI component, not the React module! label May 30, 2024
flaviendelangle
flaviendelangle reviewed Jun 3, 2024
View reviewed changes
Copy link
Member

@flaviendelangle flaviendelangle left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first glance the solution makes a lot of sense.
Thanks for taking care of it!

flaviendelangle
flaviendelangle approved these changes Jun 4, 2024
View reviewed changes
Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll let @LukasTy have the final approval here, but for me it's looking great 🥳

@LukasTy LukasTy changed the title Implements adjust for new index calculation on letter section [fields] Fix new index calculation on letter section Jun 5, 2024
LukasTy
LukasTy approved these changes Jun 5, 2024
View reviewed changes
Copy link
Member

@LukasTy LukasTy left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for finding and taking care of this issue! 🙏 💯 🎉
Great work! 👏
P.S. WDYT about adding a needs cherry-pick label to cherry-pick this fix to v6.x? 🤔

@LukasTy LukasTy added bug 🐛 Something doesn't work feature: Keyboard editing Related to the pickers keyboard edition labels Jun 5, 2024
@arthurbalduini arthurbalduini added the needs cherry-pick The PR should be cherry-picked to master after merge label Jun 5, 2024
LukasTy
LukasTy approved these changes Jun 5, 2024
View reviewed changes
packages/x-date-pickers/src/DateField/tests/editing.DateField.test.tsx
format: adapter.formats.year,
defaultValue: adapter.date('0003-01-15'),
key: 'PageDown',
expectedValue: adapter.lib === 'dayjs' ? '1898' : '9998',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flaviendelangle WDYT about making the minimum and maximum aware of the minDate and maxDate?
Didn't we do it only, because shouldDisableYear exists? 🤔
IMHO, the current behavior seems a bit strange, especially given the differences in adapters. 🙈

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would this behave for sections other than year?
For year, I do agree that having the min/max respect the validation (even if shouldDisableYear is not supported) would be a plus.
For the other fields, I'm not sure what the exact behavior should be.

@arthurbalduini arthurbalduini merged commit a2b69e3 into mui:master Jun 6, 2024
17 checks passed
github-actions bot pushed a commit that referenced this pull request Jun 6, 2024
@arthurbalduini @web-flow
[fields] Fix new index calculation on letter section (#13310)
4c63cc5
@github-actions github-actions bot mentioned this pull request Jun 6, 2024
Closed
@arthurbalduini arthurbalduini removed the needs cherry-pick The PR should be cherry-picked to master after merge label Jun 6, 2024
This was referenced Jul 12, 2024
Merged
Merged
@alhabib5565 alhabib5565 mentioned this pull request Jul 14, 2024
Open
@KotoneZQ KotoneZQ mentioned this pull request Jul 18, 2024
Open
@Carment14tham Carment14tham mentioned this pull request Jul 20, 2024
Open
DungTiger pushed a commit to DungTiger/mui-x that referenced this pull request Jul 23, 2024
@arthurbalduini @DungTiger
[fields] Fix new index calculation on letter section (mui#13310)
9e56d90
thomasmoon pushed a commit to thomasmoon/mui-x that referenced this pull request Sep 9, 2024
@arthurbalduini @thomasmoon
[fields] Fix new index calculation on letter section (mui#13310)
9ca107f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flaviendelangle flaviendelangle flaviendelangle approved these changes

@LukasTy LukasTy LukasTy approved these changes

Assignees
No one assigned
Labels
bug 🐛 Something doesn't work component: pickers This is the name of the generic UI component, not the React module! feature: Keyboard editing Related to the pickers keyboard edition
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[fields][TimeField] Adapt delta calculation for meridiem
4 participants
@arthurbalduini @mui-bot @flaviendelangle @LukasTy