Skip to content

[fields] Always use props.value as the source of truth when defined #15875

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

Merged

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Dec 13, 2024

Part of #15530

We should now have a behavior coherent with the browser input and working well on range inputs as well.
Not sure if we should count that purely as a bug fix or as a breaking change.

Follow up

  • Always use the props.value as the source of truth in usePickerValue (should be a lot simpler)

@flaviendelangle flaviendelangle added type: bug A bug or unintended behavior in the components. scope: pickers Changes or issues related to the pickers product labels Dec 13, 2024
@flaviendelangle flaviendelangle self-assigned this Dec 13, 2024
@mui-bot
Copy link

mui-bot commented Dec 13, 2024

@flaviendelangle flaviendelangle changed the title [fields] Always use props.value as the source of truth when defined [fields] Always use props.value as the source of truth when defined Dec 14, 2024
@flaviendelangle flaviendelangle force-pushed the value-field-source-of-truth branch from 169db6a to 4c89824 Compare December 18, 2024 16:08
@flaviendelangle
Copy link
Member Author

flaviendelangle commented Feb 27, 2025

As you said, the only value I see in the invalid date is for people that want to differentiate an empty date and a partially filled date.
If we have feedbacks going in this direction, we could pass a simplified version of the state.sections object in the onChange callback (or some other more direct information to say "it's partially filled"). Using the invalid date to detect this feels like relying on an implementation detail to me so migrating away from it will improve people's code eventually.

@LukasTy
Copy link
Member

LukasTy commented Feb 27, 2025

If we have feedbacks going in this direction, we could pass a simplified version of the state.sections object in the onChange callback (or some other more direct information to say "it's partially filled"). Using the invalid date to detect this feels like relying on an implementation detail to me so migrating away from it will improve people's code eventually.

Yes, thoroughly re-checking the code and thinking about it more, I've come to a similar conclusion. 👌
Let's go with simplification and once we have clear user needs - we can improve the existing solution to cater to this intermediate state. 👍
WDYT?

@flaviendelangle
Copy link
Member Author

WDYT?
I'm all in favor of going that way 👌

@flaviendelangle
Copy link
Member Author

@LukasTy the PR is updated and the test are passing 🙏

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

Looks great, but I think we could still have one "undefined" case:

  • enter any valid value
  • copy year section value
  • move to month or day section and paste the copied value

Currently this would trigger an error state with invalidDate validation.
On this PR we no longer have the error state and the onError callback returns null null as if there is no value and no error, but the field keeps the invalid section values. 🙈

Screen.Recording.2025-03-04.at.10.30.16.mov

This leads me to another question - what about the invalidDate validation?
Does/would it still make sense after this change? 🤔

@flaviendelangle
Copy link
Member Author

flaviendelangle commented Mar 4, 2025

This leads me to another question - what about the invalidDate validation?

People can still be invalid date from the outside.
I do agree it's not super frequent, but I think the validation should still support it


I'll check the regression you're describing 👍

@flaviendelangle
Copy link
Member Author

@LukasTy I did not handle fully-filled but invalid dates correctly.
For me we have two ways of handling it:

  1. We refuse the update (the section does not update)
  2. We fire an invalid date

Note: the browser input does not support copy pasting at all from what I can see, so we can't take inspiration there.
Imo, firing an invalid date is the best solution here.

@flaviendelangle flaviendelangle force-pushed the value-field-source-of-truth branch from 3a5cb50 to 0e28813 Compare March 5, 2025 06:36
@flaviendelangle flaviendelangle force-pushed the value-field-source-of-truth branch from 0e28813 to 37a5396 Compare March 5, 2025 06:44
@flaviendelangle flaviendelangle requested a review from LukasTy March 5, 2025 06:48
@LukasTy LukasTy added feature: Keyboard editing Related to the pickers keyboard edition breaking change Introduces changes that are not backward compatible. v8.x and removed type: bug A bug or unintended behavior in the components. labels Mar 5, 2025
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

I did not handle fully-filled but invalid dates correctly. For me we have two ways of handling it:

  1. We refuse the update (the section does not update)
  2. We fire an invalid date

Note: the browser input does not support copy pasting at all from what I can see, so we can't take inspiration there. Imo, firing an invalid date is the best solution here.

I think that your chosen behavior makes a lot of sense. 👍
That's also what we do currently, so, it will minimize the amount of breaking behaviors. 🙌

Could we add a migration page entry and a changelog in the PR comment? 🤔

@flaviendelangle
Copy link
Member Author

@LukasTy I added a small migration guide 👍

@flaviendelangle flaviendelangle force-pushed the value-field-source-of-truth branch from bbb3dcd to 4a57299 Compare March 5, 2025 08:40
@flaviendelangle flaviendelangle enabled auto-merge (squash) March 5, 2025 09:45
@flaviendelangle flaviendelangle merged commit 229dfea into mui:master Mar 5, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Introduces changes that are not backward compatible. feature: Keyboard editing Related to the pickers keyboard edition scope: pickers Changes or issues related to the pickers product type: regression A bug that reintroduces previously resolved issues or breaks existing functionality. v8.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants