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] Always use props.value when it changes #15490

Merged
merged 2 commits into from
Nov 20, 2024

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Nov 19, 2024

Closes #10424

@flaviendelangle flaviendelangle self-assigned this Nov 19, 2024
@flaviendelangle flaviendelangle added bug 🐛 Something doesn't work component: pickers This is the name of the generic UI component, not the React module! labels Nov 19, 2024
@@ -235,7 +235,7 @@ export const usePickerValue = <
draft: initialValue,
lastPublishedValue: initialValue,
lastCommittedValue: initialValue,
lastControlledValue: inValueWithTimezoneToRender,
lastControlledValue: inValueWithoutRenderTimezone,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to store the raw props.value variable if we want to do a reference comparison

@mui-bot
Copy link

mui-bot commented Nov 19, 2024

Deploy preview: https://deploy-preview-15490--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 8fac8d9

inValueWithTimezoneToRender,
))
) {
if (dateState.lastControlledValue !== inValueWithoutRenderTimezone) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We compare the old props.value with the new props.value (without any processing done to it)
If it changed:

  1. We set the new props.value in the state
  2. If the new derived props.value with the right timezone is not equal to the current draft, we update the draft

@flaviendelangle flaviendelangle marked this pull request as ready for review November 19, 2024 16:21
@LukasTy LukasTy added regression A bug, but worse needs cherry-pick The PR should be cherry-picked to master after merge v7.x labels Nov 19, 2024
@flaviendelangle flaviendelangle merged commit 5188453 into mui:master Nov 20, 2024
18 checks passed
@flaviendelangle flaviendelangle deleted the controlled-value-sync branch November 20, 2024 09:09
Copy link

Cherry-pick PRs will be created targeting branches: v7.x

@LukasTy
Copy link
Member

LukasTy commented Nov 20, 2024

@flaviendelangle I think that the PR only partially fixes the issue.

Essentially, we should always keep the bound value as the source of truth, even if it didn't change.

In this example, any change should not change the value as with the native input.
https://stackblitz.com/edit/react-ly6xoy?file=Demo.tsx

@flaviendelangle
Copy link
Member Author

I think we can create a dedicated issue for this exact behavior: #15530
It's not only at the picker level, it's mostly the field that works this way. I do agree that it would be nice to mimic to browser input here. The main challenge will probably be to handle invalid date correctly.
We might also have a breaking change if people do checks before storing the value in state and they don't handle invalid values correctly.

LukasTy pushed a commit to LukasTy/mui-x that referenced this pull request Dec 19, 2024
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! needs cherry-pick The PR should be cherry-picked to master after merge regression A bug, but worse v7.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pickers] Reset the value of the input field using controlled value
4 participants