-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[pickers] Rework the date lifecycle in usePickerState
#4408
[pickers] Rework the date lifecycle in usePickerState
#4408
Conversation
…isableCloseOnSelect=true
These are the results for the performance tests:
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
packages/x-date-pickers/src/MobileDatePicker/MobileDatePicker.test.tsx
Outdated
Show resolved
Hide resolved
Thanks for your work, when we can expect to have this change on an npm release? |
Most likely next week 👍 |
Which version of date picker has this change? What should be the version we need to use to render this! |
@sameswar merged pull requests are available in the next release. So it should be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is epic (with some duplicates so cheating 😛).
We would have to search deep into the project history to find a similar instance, maybe mui/material-ui#17037, but likely would need to look at 2016 or 2017 ish. I would almost want to praise it but the number of diffs makes me wonder if this could have been split into multiple PRs 😁
"closeOnSelect": { | ||
"type": { "name": "bool" }, | ||
"default": "`true` for Desktop, `false` for Mobile (based on the chosen wrapper and `desktopModeMediaQuery` prop)." | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have more context on this change? If we follow https://mui.com/material-ui/guides/api/#prop-naming. I would expect the prop to be called disableCloseOnSelect
since the desktop is more important than mobile (in terms of usage frequency).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me disableCloseOnSelect
is harder to understand than closeOnSelect
and the weight for of the desktop vs mobile does not counter this complexity.
While working on the code related to this prop, I had at least 3 times to switch the behavior because I wrote it in the wrong order.
And the doc was inversed (it was written for closeOnSelect
not disableCloseOnSelect
)
I don't think disableXXX
with a dynamic default value that can be true
or false
is a good practice. It's not readable when you don't know the codebase well enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think disableXXX with a dynamic default value that can be true or false is a good practice.
Alright, deal, sounds like we have a new rule for prop names with dynamic default. I would be in favor of having someone adding this to https://mui.com/material-ui/guides/api/#prop-naming
using Datepicker still saves the value... please help if there is any solution ? i am on mui 5.8.0 |
@kfirevron which version of |
@flaviendelangle we used the mui datepicker directly and not from x-date-picker.. maybe thats the problem ? |
You mean the pickers exported from If so yes, they are deprecated in favor of |
Hello dear developers, I am struggling for 3 days, please help |
Hi, Could you provide a working reproduction, this Codesandbox template might be a good starting point. |
OMG!!! thanks to linter, I was importing pickers from |
Hello! Having issue similar to #4438 but instead of clicking 'Cancel', clicking outside the MobileDateTimePicker triggers the onAccept event. Here is the scenario: Scenario (MobileDateTimePicker)
|
Issues
Fixes #4469
Fixes #4478
Fixes #4438
Fixes #4567
Fixes #4545
Fixes #4544
Fixes #4580
Fixes #4407
Fixes #4541
Fixes #4643
Fixes #4395
Fixes #4358
Fixes #4445
Fixes #4677
Fixes #4741
Fixes #4314
Fixes #4773
Fixes #4823
Fixes #4832
Fixes #4829
Fixes #5386
Goal
Clarify how the date should be updated when opening / closing the picker.
Changes
Behavior
onAccept
when the new accepted value equals the previous oneonChange
when the new value equals the previous onewrapperProps.onCancel
to reset to the last accepted valuedisableCloseOnSelect
prop with acloseOnSelect
prop (the default values are also inverted)Tests
MobileDatePicker
DesktopDatePicker
MobileDateTimePicker
DesktopDateTimePicker
MobileDateRangePicker
DesktopDateRangePicker
MobileTimePicker
DesktopTimePicker
What's next ?
Changelog (breaking change)
Rework the auto-closing behavior of the pickers.
The
disableCloseOnSelect
prop has been replaced by a newcloseOnSelect
prop which has the opposite behavior.The default behavior remains the same (close after the last step on desktop but not on mobile).