[pickers] Change mobile dialog close behavior #7721

LukasTy · 2023-01-26T23:28:49Z

Add resetValueOnClose property, which will reset the value to resetFallback (same as when clicking cancel) when closing modal.
Default the value to true on mobile pickers so that clicking away or Esc would reset the value (seems to make sense, given that pickers do have Cancel and Accept actions in this case)

The only relevant change in in usePickerValue file.

…close

mui-bot · 2023-01-26T23:34:23Z

	Messages
📖	Netlify deploy preview: https://deploy-preview-7721--material-ui-x.netlify.app/

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	630.6	1,004.8	649.6	794.38	167.653
Sort 100k rows	ms	716.6	1,176.8	825.5	911.2	154.732
Select 100k rows	ms	226.4	380.4	233	260.34	60.106
Deselect 100k rows	ms	147.3	357.6	195.3	211.48	75.874

Generated by 🚫 dangerJS against 362d30c

alexfauquette · 2023-01-27T13:59:00Z

packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts

 const handleDismiss = useEventCallback(() => {
 // Set all dates in state to equal the last committed date.
 // e.g. Reset the state to the last committed value.
- setDate({ value: dateState.committed, action: 'acceptAndClose' });
+ setDate({
+ value: resetValueOnClose ? dateState.resetFallback : dateState.committed,
+ action: 'acceptAndClose',
+ });
 });


I don't get why the modification is here. I agree "dismiss" is mostly used when closing UI. But I doubt it's a good idea to have onDismiss behavior depending on resetValueOnClose especially if at some point we open access to hook.

For me, it would make more sense that depending on resetValueOnClose we either call onDismiss or onCancel

Very good point. 👌

- call `handleCancel` when `resetValueOnClose`

alexfauquette

With the new naming it makes sens, @flaviendelangle do you see coming features with which it could interfere?

alexfauquette · 2023-01-30T10:48:37Z

packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts

+ * If `true`, on dismiss will reset the value to the value when the picker was opened.
+ *
+ * Might make most sense when in need of resetting the value on modal dismiss.
+ * @default `true` for mobile, `false` for desktop.


The default value of a boolean prop should be false, such that you end up with either

<Components /> or <Component resetValueOnDismiss /> but not <Component resetValueOnDismiss={false} />

Thank you for clearing this thing up. 👍
I've updated it, do you think it is now in a correct form? 🤔

flaviendelangle · 2023-01-30T11:26:29Z

The behavior of the pickers on close is a topic that keeps getting discussed, I would be very cautious before introducing any new prop, to make sure that we are solving peoples pain and not just fine tuning the use case of some specific user.

I'll try to list the issues around this topic to check 👍

LukasTy · 2023-01-30T11:30:36Z

I would be very cautious before introducing any new prop, to make sure that we are solving peoples pain and not just fine tuning the use case of some specific user.

I do understand and agree.
But I think that we either go with a complete overhaul of how the on change works (is propagated) or we try to cater to more use cases.
Would you agree that the current behaviour of accepting the selected value on picker dialog dismiss (either by clicking on backdrop or especially by clicking Esc) is somewhat weird and very opinionated. I would expect quite a lot of users wanting the acceptValueOnDismiss={false} behavior even on desktop. 🤔

flaviendelangle · 2023-01-30T12:28:44Z

Would you agree that the current behaviour of accepting the selected value on picker dialog dismiss (either by clicking on backdrop or especially by clicking Esc) is somewhat weird and very opinionated

Yes, I think people should be able to save or not when closing.

We have 2 topics here

The default behavior

Desktop

Saving when closing on desktop is what Spectrum is doing for example.
On Telerik however, the date is not filled at all before the last view is committed (by explicitly clicking on Set) so closing will reset.

Most of the discussions around this topic are linked in #4408
For example, this comment argues for the validation on close #4408 (comment)
And the team agreed at the time to save when closing on desktop.

This specific use case is probably the one with the smallest majority on any behavior.

Mobile

It is the 1st feedback we get of someone wanting to reset the value (as far as I remember).
So I don't think we should the default behavior here (and you did not propose it so I think we agree on that one).

How to let people choose another behavior

When we started to get feedback about the life-cycle, it became clear that is a very subjective topic and that if we want to make everyone happy, we need a way to customize those behaviors.
At the time, it decided to postpone the work, to let us learn better those behaviors and build something for the long term. I fear that otherwise we add half a dozen boolean props to fine tune the various life-cycle behaviors, leading to a very poor DX.

One API I had in mind was to let people override part of UsePickerValueActions, maybe with names describing actions more than behavior.

For example:

<DatePicker 
  lifecycleManager={({ setDate, dateState }) => ({ 
    onDimiss: (source: 'escape' | 'click-outside') => {
      setDate({ value: dateState.resetFallback, action: 'acceptAndClose' });
    }
  }})
/>

But that's a low level API that can easily be done in a poor way and end up being worse that all those booleans props.

In summary, it is very unclear to me what the default behaviors should be and how people should be able to override them.
I think it's a topic for which we need an umbrella issue and a description on the various use cases people a struggling with.

LukasTy · 2023-01-30T14:59:53Z

One API I had in mind was to let people override part of UsePickerValueActions, maybe with names describing actions more than behavior.

But that's a low level API that can easily be done in a poor way and end up being worse that all those booleans props.

Indeed, your suggested API does look very powerful and fully customizable, but at the same time, as you mentioned, it could be a bit too powerful and a bit too cumbersome to use for most. Unless we provide a more user friendly option.

I was also thinking about at least having a hybrid prop accepting both a boolean as well as a callback shouldAcceptValueOnDismiss: boolean | (dateState, wrapperVariant) => boolean.

Did we ever consider having hybrid props serving both a more basic case as well as a more customized one?

flaviendelangle · 2023-01-30T15:31:05Z

Did we ever consider having hybrid props serving both a more basic case as well as a more customized one?

It's not something I saw on our components, but it's something I saw on other set of components.

github-actions · 2023-04-12T08:48:25Z

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

LukasTy added 4 commits January 26, 2023 22:46

Add resetValueOnClose prop controlling if value is reset on picker …

60456b5

…close

Adjust tests

a768454

proptypes

d190c51

docs:api

6a76042

LukasTy added breaking change component: pickers This is the name of the generic UI component, not the React module! design: ux labels Jan 26, 2023

LukasTy self-assigned this Jan 26, 2023

LukasTy requested review from flaviendelangle and alexfauquette January 26, 2023 23:29

alexfauquette reviewed Jan 27, 2023

View reviewed changes

LukasTy added 4 commits January 27, 2023 16:51

Code review: Alex

e6075da

- call `handleCancel` when `resetValueOnClose`

Rename to resetValueOnDismiss to align with action name

27ebc68

proptypes

de8a923

docs:api

db5e5ed

alexfauquette reviewed Jan 30, 2023

View reviewed changes

Rename the prop to correct boolean form

362d30c

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 12, 2023

oliviertassinari added design This is about UI or UX design, please involve a designer and removed design: ux labels Aug 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pickers] Change mobile dialog close behavior #7721

[pickers] Change mobile dialog close behavior #7721

LukasTy commented Jan 26, 2023 •

edited

Loading

mui-bot commented Jan 26, 2023 •

edited

Loading

alexfauquette Jan 27, 2023

LukasTy Jan 27, 2023

LukasTy Jan 27, 2023

alexfauquette left a comment

alexfauquette Jan 30, 2023

LukasTy Jan 30, 2023

flaviendelangle commented Jan 30, 2023

LukasTy commented Jan 30, 2023

flaviendelangle commented Jan 30, 2023

LukasTy commented Jan 30, 2023

flaviendelangle commented Jan 30, 2023

github-actions bot commented Apr 12, 2023

[pickers] Change mobile dialog close behavior #7721

Are you sure you want to change the base?

[pickers] Change mobile dialog close behavior #7721

Conversation

LukasTy commented Jan 26, 2023 • edited Loading

mui-bot commented Jan 26, 2023 • edited Loading

alexfauquette Jan 27, 2023

Choose a reason for hiding this comment

LukasTy Jan 27, 2023

Choose a reason for hiding this comment

LukasTy Jan 27, 2023

Choose a reason for hiding this comment

alexfauquette left a comment

Choose a reason for hiding this comment

alexfauquette Jan 30, 2023

Choose a reason for hiding this comment

LukasTy Jan 30, 2023

Choose a reason for hiding this comment

flaviendelangle commented Jan 30, 2023

LukasTy commented Jan 30, 2023

flaviendelangle commented Jan 30, 2023

The default behavior

Desktop

Mobile

How to let people choose another behavior

LukasTy commented Jan 30, 2023

flaviendelangle commented Jan 30, 2023

github-actions bot commented Apr 12, 2023

LukasTy commented Jan 26, 2023 •

edited

Loading

mui-bot commented Jan 26, 2023 •

edited

Loading