Skip to content

[InputLabel] Remove FormLabelClasses in favor of asterisk class #14504

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Feb 14, 2019
Merged

[InputLabel] Remove FormLabelClasses in favor of asterisk class #14504

merged 6 commits into from
Feb 14, 2019

Conversation

@umairfarooq44
Copy link
Contributor

@umairfarooq44 umairfarooq44 commented Feb 12, 2019 •
edited by oliviertassinari
Loading

Fixes #14490

Breaking change

You should be able to override all the styles of the FormLabel component using the css API of the InputLabel component. We do no longer need the FormLabelClasses property.

<InputLabel
- FormLabelClasses={{ asterisk: 'bar' }}
+ classes={{ asterisk: 'bar' }}
>
  Foo
</InputLabel>

@mbrookes mbrookes changed the title [InputLabel] Asteriks class for TextField [InputLabel] Asterisk class for TextField Feb 12, 2019
@oliviertassinari oliviertassinari added the scope: text field Changes related to the text field. label Feb 12, 2019
oliviertassinari
oliviertassinari reviewed Feb 12, 2019
View reviewed changes
packages/material-ui/src/InputLabel/InputLabel.js
/**
* Override or extend the styles applied to the component.
* See [CSS API](#css-api) below for more details.
* See [CSS API](#css) below for more details.
Copy link
Member

Choose a reason for hiding this comment

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

Nice finding!

Copy link
Member

Choose a reason for hiding this comment

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

We should fix all the other links (+100)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oliviertassinari thanks for reviewing. Should I make separate PR for this?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it would be awesome!

@oliviertassinari oliviertassinari added type: new feature Expand the scope of the product to solve a new problem. PR: good for merge labels Feb 12, 2019
@oliviertassinari
Copy link
Member

Well done!

@umairfarooq44
Copy link
Contributor Author

Thanks for reviewing my PR. This is my first PR in material-ui, I would like to contribute more. Thanks for your support.

eps1lon
eps1lon reviewed Feb 13, 2019
View reviewed changes
packages/material-ui/src/InputLabel/InputLabel.js
classes,
className,
disableAnimation,
FormLabelClasses,
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change now.

@eps1lon eps1lon added the breaking change Introduces changes that are not backward compatible. label Feb 13, 2019
@eps1lon eps1lon changed the title [InputLabel] Asterisk class for TextField [InputLabel] Remove FormLabelClasses in favor of asterisk class Feb 13, 2019
eps1lon
eps1lon reviewed Feb 13, 2019
View reviewed changes
packages/material-ui/test/typescript/components.spec.tsx
FormLabelClasses={{
classes={{
root: 'foo',
asterisk: 'foo',
Copy link
Member

Choose a reason for hiding this comment

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

@pelotom This was accepted before I added the asterisk to the InputLabelClassKey. Hovering over the classes prop did show Partial<Record<InputLabelClassKey, string>> which should reject that. Writing
const classes: Partial<Record<InputLabelClassKey, string>> = { asterisk: 'foo' } did however throw an error.

I suspect this is a typescript bug?

@eps1lon eps1lon mentioned this pull request Feb 13, 2019
Closed
56 tasks
mbrookes
mbrookes previously requested changes Feb 13, 2019
View reviewed changes
packages/material-ui/src/FormLabel/FormLabel.js Outdated
filled: {},
/* Styles applied to the root element if `required={true}`. */
required: {},
/* Styles applied to the asterisk element if `required={true}`. */
Copy link
Member

@mbrookes mbrookes Feb 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
/* Styles applied to the asterisk element if `required={true}`. */
/* Styles applied to the asterisk element. */

(required just controls whether the asterisk is displayed, not whether it receives this style.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing. I have added requested changes.

mbrookes
mbrookes reviewed Feb 13, 2019
View reviewed changes
@oliviertassinari oliviertassinari merged commit 438c3a4 into mui:next Feb 14, 2019
@oliviertassinari
Copy link
Member

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

@umairfarooq44 umairfarooq44 deleted the issue-14490 branch February 14, 2019 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mbrookes mbrookes mbrookes left review comments

@oliviertassinari oliviertassinari oliviertassinari approved these changes

@eps1lon eps1lon eps1lon left review comments

Assignees

No one assigned

Labels

breaking change Introduces changes that are not backward compatible. scope: text field Changes related to the text field. type: new feature Expand the scope of the product to solve a new problem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@umairfarooq44 @oliviertassinari @mbrookes @eps1lon