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.

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

[DateTimeRangePicker] Fix date format resolving from views on 24hr locales #14341

Merged

Conversation

arthurbalduini
Copy link
Member

@arthurbalduini arthurbalduini commented Aug 26, 2024

Fixes #14334.
For DateTimeRangePickers the ["day"] case returns an incorrect format.

@arthurbalduini arthurbalduini added the component: pickers This is the name of the generic UI component, not the React module! label Aug 26, 2024
@arthurbalduini arthurbalduini self-assigned this Aug 26, 2024
@mui-bot
Copy link

mui-bot commented Aug 26, 2024

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

Generated by 🚫 dangerJS against 6f06e4e

@arthurbalduini arthurbalduini force-pushed the date-time/fix-date-format-resolver branch from 0dbc59a to dbddcbf Compare August 26, 2024 11:27
@arthurbalduini arthurbalduini changed the title [pickers] fix case for day view on DateTimePickers [pickers] Fix format for day view on DateTimeRangePickers Aug 26, 2024
@arthurbalduini arthurbalduini changed the title [pickers] Fix format for day view on DateTimeRangePickers [pickers] Fix date format on DateTimeRangePickers Aug 26, 2024
@arthurbalduini arthurbalduini changed the title [pickers] Fix date format on DateTimeRangePickers [pickers] Fix date format resolver on DateTimeRangePickers Aug 27, 2024
@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Aug 27, 2024
@@ -30,7 +35,7 @@ export const resolveDateTimeFormat = <TDate extends PickerValidDate>(
views.forEach((view) => {
if (isTimeView(view)) {
timeViews.push(view as TimeView);
} else {
} else if (isDatePickerView(view)) {
Copy link
Member

Choose a reason for hiding this comment

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

The problem was an oversight that we did not exclude meridiem view, which caused resolveDateFormat to always return keyboardDate in case of a locale with 12h clock.
On 24h clock the behavior was "correct".
I've adjusted the code to avoid this inconsistency and also added an exception to not resolve dateFormat on DateTimeRange pickers as they do not have different date views other than day for now.

@LukasTy LukasTy added component: DateTimeRangePicker The React component bug 🐛 Something doesn't work plan: Pro Impact at least one Pro user regression A bug, but worse labels Aug 29, 2024
@LukasTy LukasTy changed the title [pickers] Fix date format resolver on DateTimeRangePickers [DateTimeRangePicker] Fix date format resolving from views on 24hr locales Aug 29, 2024
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

I've adjusted the approach to one that looks more correct to me.
I'm not sure why we would have differentiated the behavior based on the presence of value. 🤔

Let me know what you think about the solution. 😉

@arthurbalduini
Copy link
Member Author

I've adjusted the approach to one that looks more correct to me. I'm not sure why we would have differentiated the behavior based on the presence of value. 🤔

Let me know what you think about the solution. 😉

I assumed that veryfing if the value is an array (date range) would be more clear than adding a new parameter to the function to be used only in DateTimePickers, since the view verification was done in resolveDateFormat().
Let's go with your solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: DateTimeRangePicker The React component component: pickers This is the name of the generic UI component, not the React module! plan: Pro Impact at least one Pro user regression A bug, but worse size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
3 participants