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] Reset the value of the input field using controlled value #10424

Open
satwinder8294 opened this issue Sep 21, 2023 · 16 comments
Open
Labels
bug 🐛 Something doesn't work component: pickers This is the name of the generic UI component, not the React module! regression A bug, but worse

Comments

@satwinder8294
Copy link

satwinder8294 commented Sep 21, 2023

Steps to reproduce 🕹

Link to live example:

Steps:
1.
2.
3.

Current behavior 😯

I am using in my React + MUI application... My calendar date selections are working fine as expected. But the user is allowed to type invalid dates in the input field e.g 31st of November or 13th month of the year. if user types an invalid value in the input field then instead of updating the state value my code reset it to previous value by using the inputRefs.

It changes the value to previous one but when i open the calendar it again change the value(in input field) to invalid date.

Can you help how to handle this ?

Expected behavior 🤔

Expectation is that if the date is invalid, set it back to valid one programmatically both in calendar and the input field.

Context 🔦

as a work around, i have used key attribute and re-render on each invalid date. I would like to get a proper solution.

Your environment 🌎

npx @mui/envinfo
  Don't forget to mention which browser you used.
  Output from `npx @mui/envinfo` goes here.

Order ID or Support key 💳 (optional)

No response

@satwinder8294 satwinder8294 added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 21, 2023
@zannager zannager added the component: pickers This is the name of the generic UI component, not the React module! label Sep 21, 2023
@LukasTy
Copy link
Member

LukasTy commented Sep 22, 2023

Thank you for creating this issue. 🙏
A reproduction example would have been great to better understand your problem, but I think I get the idea and see the problem.
The problem is that currently there is no way to tell the picker to use the bound value, because of this: https://github.com/mui/mui-x/blob/master/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts#L313

A possible solution could be to add an optional ref equality check:

diff --git a/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts b/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts
index 6cb85178f..7a9b504c9 100644
--- a/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts
+++ b/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts
@@ -310,7 +310,8 @@ export const usePickerValue = <
   if (
     inValue !== undefined &&
     (dateState.lastControlledValue === undefined ||
-      !valueManager.areValuesEqual(utils, dateState.lastControlledValue, inValue))
+      !valueManager.areValuesEqual(utils, dateState.lastControlledValue, inValue) ||
+      dateState.lastControlledValue !== inValue)
   ) {
     const isUpdateComingFromPicker = valueManager.areValuesEqual(utils, dateState.draft, inValue);

This would allow re-setting the picker value when the bound value reference changes, i.e. by doing setValue(currentValue => dayjs(currentValue)).

Are there any problems that I don't see with this or better suggestions? 🤔

@LukasTy LukasTy added enhancement This is not a bug, nor a new feature and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Sep 22, 2023
@LukasTy LukasTy changed the title [DesktopDatePicker] Reset the value of the input field using defaultValue or value [pickers] Reset the value of the input field using defaultValue or value Sep 22, 2023
@satwinder8294
Copy link
Author

Thank you for creating this issue. 🙏 A reproduction example would have been great to better understand your problem, but I think I get the idea and see the problem. The problem is that currently there is no way to tell the picker to use the bound value, because of this: https://github.com/mui/mui-x/blob/master/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts#L313

A possible solution could be to add an optional ref equality check:

diff --git a/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts b/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts
index 6cb85178f..7a9b504c9 100644
--- a/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts
+++ b/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts
@@ -310,7 +310,8 @@ export const usePickerValue = <
   if (
     inValue !== undefined &&
     (dateState.lastControlledValue === undefined ||
-      !valueManager.areValuesEqual(utils, dateState.lastControlledValue, inValue))
+      !valueManager.areValuesEqual(utils, dateState.lastControlledValue, inValue) ||
+      dateState.lastControlledValue !== inValue)
   ) {
     const isUpdateComingFromPicker = valueManager.areValuesEqual(utils, dateState.draft, inValue);

This would allow re-setting the picker value when the bound value reference changes, i.e. by doing setValue(currentValue => dayjs(currentValue)).

Are there any problems that I don't see with this or better suggestions? 🤔

hi @LukasTy , Thank you for quick response. I have already tried to set the state like this but that didn't help. Do I need to make any other change as well except this setState ?

If you want me to make a change in some package file, that wont help, because before each deployment on server, it install the npm modules again,, so my change will be lost

@LukasTy
Copy link
Member

LukasTy commented Sep 22, 2023

Do I need to make any other change as well except this setState ?

If you want me to make a change in some package file, that wont help, because before each deployment on server, it install the npm modules again,, so my change will be lost

My question was more open-ended and targeted to any other team member who would take a look at this issue.
I've moved the issue into the needs grooming state so that we can address it and come up with the best solution for this case. 😉
In the meantime, if you really need this functionality and want to "battle-test" it, you can use https://www.npmjs.com/package/patch-package to apply my mentioned change (diff) to your installation.
Just confirming, that It will also run on CI. 😉

@satwinder8294
Copy link
Author

Do I need to make any other change as well except this setState ?
If you want me to make a change in some package file, that wont help, because before each deployment on server, it install the npm modules again,, so my change will be lost

My question was more open-ended and targeted to any other team member who would take a look at this issue. I've moved the issue into the needs grooming state so that we can address it and come up with the best solution for this case. 😉 In the meantime, if you really need this functionality and want to "battle-test" it, you can use https://www.npmjs.com/package/patch-package to apply my mentioned change (diff) to your installation. Just confirming, that It will also run on CI. 😉

Thank you. Really amazed by your quick responses. I am sorry, but I don't know how this patch thing work and I am afraid of sending it to the production. Is there any way that we can stop invalid dates in the textbox itself ? I mean not allow user to enter invalid numbers such as 13th month or 31st of November etc. ? Then we won't even need to reset it through refs.

@LukasTy
Copy link
Member

LukasTy commented Sep 25, 2023

Is there any way that we can stop invalid dates in the textbox itself ? I mean not allow user to enter invalid numbers such as 13th month or 31st of November etc. ? Then we won't even need to reset it through refs.

The initial version of fields did work like that, but it was changed with #7934
The issue is that the dependendant sections is not something that is very accessibility friendly and on top of that, it's not how the native html date input works, that's why we decided not to re-invent a wheel on this regard.

@satwinder8294
Copy link
Author

Thank you for creating this issue. 🙏 A reproduction example would have been great to better understand your problem, but I think I get the idea and see the problem. The problem is that currently there is no way to tell the picker to use the bound value, because of this: https://github.com/mui/mui-x/blob/master/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts#L313
A possible solution could be to add an optional ref equality check:

diff --git a/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts b/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts
index 6cb85178f..7a9b504c9 100644
--- a/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts
+++ b/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts
@@ -310,7 +310,8 @@ export const usePickerValue = <
   if (
     inValue !== undefined &&
     (dateState.lastControlledValue === undefined ||
-      !valueManager.areValuesEqual(utils, dateState.lastControlledValue, inValue))
+      !valueManager.areValuesEqual(utils, dateState.lastControlledValue, inValue) ||
+      dateState.lastControlledValue !== inValue)
   ) {
     const isUpdateComingFromPicker = valueManager.areValuesEqual(utils, dateState.draft, inValue);

This would allow re-setting the picker value when the bound value reference changes, i.e. by doing setValue(currentValue => dayjs(currentValue)).
Are there any problems that I don't see with this or better suggestions? 🤔

hi @LukasTy , Thank you for quick response. I have already tried to set the state like this but that didn't help. Do I need to make any other change as well except this setState ?

If you want me to make a change in some package file, that wont help, because before each deployment on server, it install the npm modules again,, so my change will be lost

okay, is there any plan to merge this fix as well ?

@LukasTy
Copy link
Member

LukasTy commented Sep 26, 2023

okay, is there any plan to merge this fix as well ?

I'll try creating a PR this week. 👌

@LukasTy LukasTy changed the title [pickers] Reset the value of the input field using defaultValue or value [pickers] Reset the value of the input field using or value Sep 26, 2023
@satwinder8294
Copy link
Author

okay, is there any plan to merge this fix as well ?

I'll try creating a PR this week. 👌

Thank you very much. Will keep an eye :)

@LukasTy LukasTy changed the title [pickers] Reset the value of the input field using or value [pickers] Reset the value of the input field using controlled value Oct 2, 2023
@alexfauquette alexfauquette added bug 🐛 Something doesn't work and removed enhancement This is not a bug, nor a new feature labels Oct 3, 2023
@gitstart
Copy link
Contributor

gitstart commented Oct 9, 2023

@satwinder8294 @LukasTy we would like to pick this up

@LukasTy
Copy link
Member

LukasTy commented Oct 9, 2023

@gitstart Thank you for being interested in contributing to the Pickers codebase! 🙏
Do you have a rough idea for how difficult this problem would be?
We feel that this could be a pretty tricky problem to tackle. On top of that, it's the most integral behavioral part of the components.
If you are not afraid of such challenge, feel free to explore a solution! 👍
But there also are other issues that are more clear and also pretty important.
Have you considered #9956 or #7633, which is potentially a bit more tricky given our v7 plans. 🙈

@gitstart
Copy link
Contributor

gitstart commented Oct 9, 2023

Thanks for getting back to us @LukasTy, we are willing to work on as many of these issues as you like.

@LukasTy
Copy link
Member

LukasTy commented Oct 20, 2023

Another case to test and cover when fixing this: #10727.

@gbataille
Copy link

I just hit the same problem. I want to limit the range used to 3 years, so I want to programatically control the value where I basically force the end date to be a maximum of 3 years from the start date.

Any idea when we can expect that fixed? also, may I suggest that if it's difficult to fix now, you put a note in the doc saying that there is currently no "controlled" version of the component? I needed quite a bit of time to figure out that I was doing things right and that there was a bug in the library

Thanks

@LukasTy
Copy link
Member

LukasTy commented Jun 27, 2024

@gbataille given your use case, a maxDate validation reliant on the selected start date comes to mind.
Have you tried such validation? 🤔

Forcibly replacing the changed value with the one you need sounds like a bit strange UX.

I think that relying on form validation and preventing form submission with validation errors present is a better UX for both regular users as well as visually impaired relying on a11y features.

As for the issue, thank you for bumping it, we put it in the To explore board, which has lower priority than the To do board. 🙈
I've adjusted this. 👌

@gbataille
Copy link

gbataille commented Jun 27, 2024

I quite agree for the UX side of what I'm doing (I mean that as you say it's not awesome).

I did not think to change the maxDate dynamically. I guess it gives me something interesting indeed, based on which I could avoid going further. I'll investigate what's possible.
Thanks for the pointer.

@LukasTy
Copy link
Member

LukasTy commented Aug 6, 2024

We agreed to work on aligning the controlled behavior as close to the regular <input type="date" /> as possible.
This would mean—not changing the value in the input if onChange does not update the value when editing in both the field as well as the Picker view (i.e. calendar).

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: pickers This is the name of the generic UI component, not the React module! regression A bug, but worse
Projects
None yet
Development

No branches or pull requests

6 participants