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

[DatePicker] Fix previous input value issues #4383

Closed
wants to merge 4 commits into from

Conversation

yyq1025
Copy link

@yyq1025 yyq1025 commented Apr 6, 2022

Re-open mui/material-ui#31929 here

fixes #4478, fixes #4469

not sure whether mui/material-ui#4358 is really a bug. I think people need a way to cancel their selections on the desktop.

#4478 and #4469 are caused by calling setInitialDate whenever we close the picker. This design could not handle the input change after closing. Instead, we should call setInitialDate just before the picker opens.

@mui-bot
Copy link

mui-bot commented Apr 6, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 262.9 467.4 326 331.14 74.251
Sort 100k rows ms 484.5 895.5 689.1 724.86 155.216
Select 100k rows ms 104.9 208.6 161.4 161.14 37.175
Deselect 100k rows ms 123.1 230.2 192.9 188.18 39.156

Generated by 🚫 dangerJS against cd3e46e

@flaviendelangle
Copy link
Member

@yyq321 for mui/material-ui#4358, on desktop, when using disableCloseOnSelect, how can you validate your data if closing the picker reset its value ?

@yyq1025
Copy link
Author

yyq1025 commented Apr 8, 2022

@flaviendelangle thanks for reminding me. disableCloseOnSelect is a complex problem when considering for both mobile and desktop. Therefore, I enumerate all possible situations and expected behaviors based on my understanding of pickers:

  • On mobile and disableCloseOnSelect is true: clicking the cancel button or clicking away (onDismiss) should reset the date to initialDate and close pickers
  • On mobile and disableCloseOnSelect is false: the same as above, and close picker immediately after selecting a date
  • On desktop and disableCloseOnSelect is true: clicking away (onDismiss) should accept the selected date now and close pickers. That is, onDismiss and onAccept should have the same effect here
  • On desktop and disableCloseOnSelect is false: clicking away (onDismiss) should reset the date to initialDate and close pickers

Therefore, on mobile or on desktop and disableCloseOnSelect is false, onDismiss should reset the date. In the other case, onDismiss should accept the date.

@flaviendelangle
Copy link
Member

flaviendelangle commented Apr 11, 2022

@yyq321 thanks for your detailed feedback

I am trying to clarify this behavior in mui/material-ui#4408
I agree that it is a key behavior and that any change should be done carefully

@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
Copy link

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

@BGehrels
Copy link

  • On desktop and disableCloseOnSelect is false: clicking away (onDismiss) should reset the date to initialDate and close pickers

I'm not sure about this: This feels nice when you first select a date, but when you open a picker for the second time, you probably want to correct an error you did the first time. "Open-> fix day -> click away -> picker closes" ist a very convinient way and feels "natural". "Open -> fix the day -> reclick the selected hour -> reclick the selected minute -> picker closes" is annoying.

I would also argue that there is no use case for cancelling an accidential selection on desktop, as you can always change it. Cancelling feels more like a 90ies Wizard UX than a modern immediate-visual-feedback-change-as-you-go approach

Last but not least: usually, the value will be shown in the connected TextArea and be updated immediatly while you change the pickers, as shown in the second video here. So the UX metaphor is "selecting is equivalent to typing", as the field updates the same way as if i type in there. But: No TextArea ever clears/resets when i click away. So neither should the pickers selection.

@flaviendelangle
Copy link
Member

flaviendelangle commented Apr 11, 2022

@BGehrels can we continue this discusson on mui/material-ui#4408 ?
I will document correctly the usePickersState hook. These reset represent like a third of the recent issues around the pickers so let's clean it correctly

@alexfauquette
Copy link
Member

alexfauquette commented Apr 12, 2022

@flaviendelangle do you mean PR #4408 instead of the issue mui/material-ui#4408 ?

@flaviendelangle
Copy link
Member

Closing this one if favor of #4408

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
5 participants