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

Change instances of FormToggle to use ToggleControl component #4996

Conversation

paulwilde
Copy link
Contributor

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.

…ch includes the field label and handles the instance ID automatically.
<label key="label" htmlFor={ commentsToggleId }>{ __( 'Allow Comments' ) }</label>,
<FormToggle
return (
<ToggleControl
key="toggle"
Copy link
Member

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.

https://reactjs.org/docs/lists-and-keys.html

<FormToggle
id={ pendingId }
<ToggleControl
key="toggle"
Copy link
Member

Choose a reason for hiding this comment

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

return [
<label key="label" htmlFor={ pingbacksToggleId }>{ __( 'Allow Pingbacks & Trackbacks' ) }</label>,
<FormToggle
<ToggleControl
key="toggle"
Copy link
Member

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.

https://reactjs.org/docs/lists-and-keys.html

}

.blocks-base-control:last-child,
.blocks-toggle-control > .blocks-base-control__label {
Copy link
Member

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 within PanelRow have their bottom margin removed?
  • Should it be that all ToggleControl labels have their bottom margin removed? (applied in toggle-control/style.scss)

Copy link
Contributor Author

@paulwilde paulwilde Feb 19, 2018

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.

Copy link
Member

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?

Copy link
Contributor Author

@paulwilde paulwilde Feb 22, 2018

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think applying globally .blocks-base-control:last-child { margin-bottom: 0 } would make sense, as currently the last control in a series of controls does add some updates unnecessary whitespace.

screen shot 2018-02-22 at 12 32 47

@danielbachhuber danielbachhuber added the [Status] Needs More Info Follow-up required in order to be actionable. label Jun 15, 2018
@danielbachhuber
Copy link
Member

@paulwilde Are you still planning to pursue this further?

@aduth
Copy link
Member

aduth commented Sep 13, 2018

Closing as stale without response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Needs More Info Follow-up required in order to be actionable.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants