-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[FormControlLabel] Add labelPlacement prop #12303
[FormControlLabel] Add labelPlacement prop #12303
Conversation
@@ -74,6 +79,7 @@ function FormControlLabel(props, context) { | |||
className={classNames( | |||
classes.root, | |||
{ | |||
[classes.start]: labelPlacement === 'start', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What're the pros/cons of using CSS over changing the DOM structure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pros: It's simpler (so simple that it barely warrants its own prop).
Cons: ? (I'm not aware of any)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cons: confusing output when debugging in the dev tool. But I'm fine with the current tradeoff.
@@ -22,6 +22,10 @@ export const styles = theme => ({ | |||
cursor: 'default', | |||
}, | |||
}, | |||
/* Styles applied to the root element if `position="start"`. */ | |||
start: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of?
The variants applied by and enum property are prefixed, e.g. the colorPrimary class applied by the color="primary" property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of renaming the start
style rule, labelPositionStart
?
@@ -22,6 +22,10 @@ export const styles = theme => ({ | |||
cursor: 'default', | |||
}, | |||
}, | |||
/* Styles applied to the root element if `position="start"`. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
labelPositition="start"
Closes #11618
I reused the RadioButtonsGroup demo to show the label on the left, and moved the FromControl
required
anderror
props examples to the CheckboxesGroup example. (They never really made sense on RadioButtons.)