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

[FormHelperText] Error styles should override disabled styles #13217

Merged

Conversation

matthewdordal
Copy link
Contributor

@matthewdordal matthewdordal commented Oct 12, 2018

Material UI is a great project so just looking to help out.

According to the issue the error styles in the FormHelperText take precedence over the disabled styles. Please let me know if there is further changes that are required. If this is not an actual issue also feel free to close the PR.

Before this PR here was the behavior:

Example Code:

<TextField
          error
          disabled
          id="standard-disabled"
          label="Disabled"
          defaultValue="Hello World"
          className={classes.textField}
          margin="normal"
          helperText="Some helper text"
 />

Before this PR:

image

After this PR:

image

Closes #13210

@mbrookes
Copy link
Member

@matthewdordal Please could you follow the issue title / commit message format we use elsewhere?

"[ComponentName] Imperative commit message"

@matthewdordal matthewdordal changed the title fix(FormHelperText): error styles override disabled FormHelperText: error styles should override disabled styles Oct 12, 2018
@matthewdordal matthewdordal changed the title FormHelperText: error styles should override disabled styles [FormHelperText] Error styles should override disabled styles Oct 12, 2018
@matthewdordal matthewdordal force-pushed the issue-13210/TextField-helper-text branch from 84788b8 to 56d1991 Compare October 12, 2018 15:02
@matthewdordal
Copy link
Contributor Author

@mbrookes sorry that I missed the commit message format. I updated the commit message. Please let me know if there's any other things I should update. 😸

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

I can confirm the same behavior with Bootstrap. It makes sense. In some way, this change can be seen as breaking, but I like to think it's a bug fix, solving a usability problem.
capture d ecran 2018-10-13 a 00 33 51

@oliviertassinari oliviertassinari added the component: text field This is the name of the generic UI component, not the React module! label Oct 12, 2018
@oliviertassinari
Copy link
Member

Let's wait for the next minor v3.3.0.

@oliviertassinari oliviertassinari added this to the v3.3.0 milestone Oct 12, 2018
@mbrookes
Copy link
Member

mbrookes commented Oct 12, 2018

@oliviertassinari I agree with you that this is more of a bugfix, however I'm not sure how much sense it makes (the behavior, rather than the fix per se). How can you fix an error on a disabled input?!

@oliviertassinari oliviertassinari added the new feature New feature or request label Oct 12, 2018
@jiayuandeng
Copy link
Contributor

jiayuandeng commented Oct 15, 2018

@mbrookes The error could be fixed by another component. Here is a case:

Assume a user can use the Select (<TextField select />) to select from some uploaded files (like using file IDs). If there is no file to select, the Select is disabled, and an error message shows when the user tries to submit the form. To fix the error, the user can click a button next to it to upload a file. Once a file is uploaded, the select is not disabled anymore.

@oliviertassinari oliviertassinari merged commit b784369 into mui:master Oct 16, 2018
@oliviertassinari
Copy link
Member

@matthewdordal It's a great first pull request on Material-UI 👌🏻. Thank you for working on it!

@matthewdordal matthewdordal deleted the issue-13210/TextField-helper-text branch October 25, 2018 16:28
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! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants