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

[pickers] Rework the date lifecycle in usePickerState #4408

Merged
merged 85 commits into from
Apr 29, 2022

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Apr 8, 2022

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

    • Do not call onAccept when the new accepted value equals the previous one
    • Do not call onChange when the new value equals the previous one
    • Commit value when dismissing the picker
    • Create new wrapperProps.onCancel to reset to the last accepted value
    • Replace the disableCloseOnSelect prop with a closeOnSelect prop (the default values are also inverted)
  • Tests

    • MobileDatePicker
    • DesktopDatePicker
    • MobileDateTimePicker
    • DesktopDateTimePicker
    • MobileDateRangePicker
    • DesktopDateRangePicker
    • MobileTimePicker
    • DesktopTimePicker

What's next ?

  • Clean the other tests to always use the new utilities
  • Test the lifecyle of static pickers

Changelog (breaking change)

  • Rework the auto-closing behavior of the pickers.

    The disableCloseOnSelect prop has been replaced by a new closeOnSelect prop which has the opposite behavior.
    The default behavior remains the same (close after the last step on desktop but not on mobile).

    // If you don't want to close after the last step
    -<DatePicker disableCloseOnSelect={false} />
    +<DatePicker closeOnSelect />
    
    // If you want to close after the last step
    -<DatePicker disableCloseOnSelect />
    +<DatePicker closeOnSelect={false} />

@flaviendelangle flaviendelangle self-assigned this Apr 8, 2022
@flaviendelangle flaviendelangle added bug 🐛 Something doesn't work component: pickers This is the name of the generic UI component, not the React module! labels Apr 8, 2022
@mui-bot
Copy link

mui-bot commented Apr 8, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 249.8 423.4 336.8 335.86 67.196
Sort 100k rows ms 433.9 823.5 706.9 655.42 134.787
Select 100k rows ms 100.5 214.2 164.7 162.46 40.569
Deselect 100k rows ms 108.4 211.1 176.5 160.58 40.078

Generated by 🚫 dangerJS against d3ab2c7

@flaviendelangle flaviendelangle marked this pull request as draft April 8, 2022 12:49
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 9, 2022
@github-actions
Copy link

github-actions bot commented Apr 9, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 11, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 11, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 11, 2022
@flaviendelangle flaviendelangle merged commit 9ae983f into mui:master Apr 29, 2022
@flaviendelangle flaviendelangle deleted the disableCloseOnSelect-value branch April 29, 2022 12:15
@SaeedZhiany
Copy link
Contributor

@flaviendelangle

Thanks for your work, when we can expect to have this change on an npm release?

@flaviendelangle
Copy link
Member Author

Most likely next week 👍

@sameswar
Copy link

sameswar commented May 4, 2022

Which version of date picker has this change? What should be the version we need to use to render this!

@alexfauquette
Copy link
Member

@sameswar merged pull requests are available in the next release. So it should be @5.0.0-alpha.3 the day of the next release is not yet decided, probably in less than a week

Copy link
Member

@oliviertassinari oliviertassinari left a 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 😛).

Screenshot 2022-05-15 at 02 17 48

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 😁

Comment on lines +14 to +17
"closeOnSelect": {
"type": { "name": "bool" },
"default": "`true` for Desktop, `false` for Mobile (based on the chosen wrapper and `desktopModeMediaQuery` prop)."
},
Copy link
Member

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).

Copy link
Member Author

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.

Copy link
Member

@oliviertassinari oliviertassinari May 23, 2022

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

@kfirevron
Copy link

using Datepicker still saves the value...
steps to reproduce :
1.click on day sets the value.
2.closing popup.
3.click clear icon and set value to null.
4.click on open popup.
5.click outside the popup old value showing in the input.

please help if there is any solution ? i am on mui 5.8.0

@flaviendelangle
Copy link
Member Author

@kfirevron which version of @mui/x-date-picker are you using ?

@kfirevron
Copy link

@flaviendelangle we used the mui datepicker directly and not from x-date-picker.. maybe thats the problem ?

@flaviendelangle
Copy link
Member Author

You mean the pickers exported from @mui/lab ?

If so yes, they are deprecated in favor of @mui/x-date-picker and will be removed soon
You have more details about this migration here

@igorSurz
Copy link

igorSurz commented Oct 12, 2022

Hello dear developers, I am struggling for 3 days, please help
"@mui/x-date-pickers-pro": "^5.0.4",
The date time picker reverts its initial state if we click away, I have read all these topics and the developers marked this bug as solved😨

@flaviendelangle
Copy link
Member Author

flaviendelangle commented Oct 12, 2022

Hi,

Could you provide a working reproduction, this Codesandbox template might be a good starting point.

@igorSurz
Copy link

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 @lab and not from @mui/x-date-pickers/
@flaviendelangle Thank you, my friend, I wish you to live to 120 years🙏🏻🙏🏻🙏🏻

@djwondersco
Copy link

djwondersco commented Nov 22, 2022

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)

  1. Open the picker (no log)
  2. Click on a date (onChange)
  3. Click on a hour (onChange)
  4. Click on a minute (onChange)
  5. Click on outside the picker (backdrop of the modal)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment