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

[docs] Simplify state management in Text Field demo page #35051

Merged
merged 54 commits into from Dec 12, 2022

Conversation

PratikDev
Copy link
Contributor

@PratikDev PratikDev commented Nov 8, 2022

Fixes #35041

Text Field demo page had several components that weren't managing the state perfectly. some had synced values and some didn't. Since these demos' main purpose isn't to show the usage of controlled vs. uncontrolled modes, I converted all of them to uncontrolled to reduce confusion

managing state of MultilineTextFields component on multiline-flexible. so that every input field has a different state

Signed-off-by: PratikDev <87067570+PratikDev@users.noreply.github.com>
managing state of SelectTextFields component. so that every input field has a different state

Signed-off-by: PratikDev <87067570+PratikDev@users.noreply.github.com>
managing state of ComposedTextField component. so that every input field has a different state

Signed-off-by: PratikDev <87067570+PratikDev@users.noreply.github.com>
Signed-off-by: PratikDev <87067570+PratikDev@users.noreply.github.com>
Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
added state so all fields have synced values. used FormControl here, so that all fields have placeholder's floating animation when one is changed

Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
added state so that all fields have synced values

Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
added state so all fields have synced values

Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
added state so all fields have synced values

Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
added state so all fields have synced values

Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
added state so all fields have synced values

Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
used the unused state "weightRange" as With Normal TextField's state. 

Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
added state so that all fields have synced values

Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
added state so that all fields have synced values

Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
added state so that all fields have synced values

Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
added state so that all fields have synced values

Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
added state so that all fields have synced values

Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
added state so that all fields have synced values

Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
added state so that all fields have synced values

Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
added state so that all fields have synced values

Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
added state so that all fields have synced values

Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
added state so that all fields have synced values

Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
@mui-bot
Copy link

mui-bot commented Nov 8, 2022

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

No bundle size changes

Generated by 🚫 dangerJS against f713eeb

@zannager zannager added docs Improvements or additions to the documentation component: text field This is the name of the generic UI component, not the React module! labels Nov 9, 2022
applied eslint and prettier

Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
applied eslint and prettier

Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
@michaldudak
Copy link
Member

This PR makes all the components controlled. We've discussed making them uncontrolled instead to simplify the code and make the demos more focused on what their primary purpose is.

@PratikDev
Copy link
Contributor Author

oh boyy. i guess i got some misconception. did you mean stateless by "uncontrolled"?

@michaldudak
Copy link
Member

https://reactjs.org/docs/uncontrolled-components.html

Basically, components without the value prop set (so they are in charge of controlling their own state changes).

@PratikDev
Copy link
Contributor Author

understood. as this PR has gone so blurry it could be confusing for anyone. so should I just delete this and create a new one with a more perfect description? or just commit the edits in it?

@PratikDev
Copy link
Contributor Author

hey @michaldudak, haven't received any feedback about this PR for a long time

Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. With so many PRs and issues, it's hard to prioritize.
I left two comments, but it looks OK in general.

@@ -50,8 +35,7 @@ export default function InputAdornments() {
<FormControl sx={{ m: 1, width: '25ch' }} variant="outlined">
<OutlinedInput
id="outlined-adornment-weight"
value={values.weight}
onChange={handleChange('weight')}
defaultValue=""
Copy link
Member

Choose a reason for hiding this comment

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

Empty defaultValue is not necessary.

@@ -24,8 +18,7 @@ export default function MultilineTextFields() {
label="Multiline"
multiline
maxRows={4}
value={value}
onChange={handleChange}
defaultValue="Controlled"
Copy link
Member

Choose a reason for hiding this comment

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

It's not controlled anymore. You can remove the defaultValue at all IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so should remove the defaultValue from all the input fields?

Copy link
Member

Choose a reason for hiding this comment

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

In cases where it's "Controlled" now, yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are we again having some confusion about"controlled" and "uncontrolled" by any chance? because I think I made all of the fields "uncontrolled" as per our discussion

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's correct; all the fields are uncontrolled. That's why having a defaultValue="Controlled" is confusing and I'm asking you to remove the defaultValue prop where its value is "Controlled" (literally).

@michaldudak michaldudak changed the title [docs] fix state management in Text Field demo page [docs] Simplify state management in Text Field demo page Dec 6, 2022
@PratikDev
Copy link
Contributor Author

sorry for all the confusion we had. but seems we're now on the same page. please check out my recent commit. I've removed all the defaultValue="Controlled" and defaultValue=""

showPassword: !values.showPassword,
});
};
const handleClickShowPassword = () => setShowPassword(!showPassword);
Copy link
Member

Choose a reason for hiding this comment

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

Changing state based on a previous value should use the callback form:

Suggested change
const handleClickShowPassword = () => setShowPassword(!showPassword);
const handleClickShowPassword = () => setShowPassword((show) => !show);

@@ -59,8 +35,7 @@ export default function InputAdornments() {
<FormControl sx={{ m: 1, width: '25ch' }} variant="outlined">
<OutlinedInput
id="outlined-adornment-weight"
value={values.weight}
onChange={handleChange('weight')}
defaultValue=""
Copy link
Member

Choose a reason for hiding this comment

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

You can remove empty defaultValue props here as well

@@ -24,8 +18,7 @@ export default function MultilineTextFields() {
label="Multiline"
multiline
maxRows={4}
value={value}
onChange={handleChange}
defaultValue="Controlled"
Copy link
Member

Choose a reason for hiding this comment

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

Remove the defaultValue props

Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
Signed-off-by: Pratik Dev <87067570+PratikDev@users.noreply.github.com>
@PratikDev
Copy link
Contributor Author

Hi, I think I removed all the defaultValue="" and defaultValue="Controlled". and also changed the state updating in a callback form. There are no more defaultValue="" and defaultValue="Controlled" in any text fields as far I checked. but if there's anything more to change, please let me know. I'll commit them accordingly

Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

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

Looks good to me now. Thanks for your work!

@PratikDev
Copy link
Contributor Author

the PR still shows Open. is there anything more to do from my end? or I'm such a noob in open source who doesn't even know how things work here?

@michaldudak
Copy link
Member

No, no need to do anything :) It should be included in the next release. We leave PRs open for some time after approval in case other MUI team members would like to comment.

@PratikDev
Copy link
Contributor Author

PratikDev commented Dec 9, 2022 via email

@michaldudak michaldudak merged commit 846bd9b into mui:master Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: text field This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[docs] many text fields don't manage separate state in the demo and several get changed when one is changed
4 participants