-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Change instances of FormToggle to use ToggleControl component #4996
Change instances of FormToggle to use ToggleControl component #4996
Conversation
…ch includes the field label and handles the instance ID automatically.
<label key="label" htmlFor={ commentsToggleId }>{ __( 'Allow Comments' ) }</label>, | ||
<FormToggle | ||
return ( | ||
<ToggleControl | ||
key="toggle" |
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.
This key
attribute is no longer necessary since we're not returning an array.
<FormToggle | ||
id={ pendingId } | ||
<ToggleControl | ||
key="toggle" |
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.
This key
is not necessary.
return [ | ||
<label key="label" htmlFor={ pingbacksToggleId }>{ __( 'Allow Pingbacks & Trackbacks' ) }</label>, | ||
<FormToggle | ||
<ToggleControl | ||
key="toggle" |
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.
This key
attribute is no longer necessary since we're not returning an array.
} | ||
|
||
.blocks-base-control:last-child, | ||
.blocks-toggle-control > .blocks-base-control__label { |
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.
I'm concerned how this is tying toggle control to panel components, and am wondering if the desired goal here could be generalized more.
Two thoughts of alternatives:
- Should it be that all
.blocks-base-control__label
withinPanelRow
have their bottom margin removed? - Should it be that all
ToggleControl
labels have their bottom margin removed? (applied intoggle-control/style.scss
)
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.
The general problem (which #4997 also has) is that .blocks-base-control
is forcing a margin at the bottom of each control. There isn't any utility class which removes that margin forcing it to be globally removed in this instance, where each control is stacked without any space between.
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.
Right, but should it be that one or the other of .blocks-base-control
and .blocks-base-control__label
applies a bottom margin, not both?
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.
Removing the bottom margin from .blocks-base-control__label
is due to their being no bottom margin on these controls as they exist currently (For these controls I'm changing). They have a margin-bottom: 5px
when used for block inspector controls, but to not change how they look in this case I removed the margin.
It wouldn't make sense to remove that margin for all controls, as having some margin between the label and input/textarea controls makes sense.
Maybe opting to globally remove the label margin for toggle controls would be the best way forward?
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.
@paulwilde Are you still planning to pursue this further? |
Closing as stale without response. |
Inspired by #4817, this moves all instances of the FormToggle component to use the ToggleControl component. It also adds a showHint prop to the ToggleControl.