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.

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

Change instances of FormToggle to use ToggleControl component #4996

Closed
wants to merge 4 commits into from
Closed

Change instances of FormToggle to use ToggleControl component #4996

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
abe96b3
Update all instances of FormToggle to use ToggleControl component whi…
Feb 10, 2018
40e2aca
Add showHint prop to documentation.
Feb 10, 2018
5016de7
Fix build error.
Feb 11, 2018
174dd27
Remove key attribute.
Feb 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions components/panel/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,15 @@
&:empty {
margin-top: 0;
}

.blocks-base-control {
width: 100%;
}

.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 •
edited
Loading

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 •
edited
Loading

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

margin-bottom: 0;
}
}

.components-panel .circle-picker {
Expand Down
6 changes: 6 additions & 0 deletions components/toggle-control/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,9 @@ A function that receives the checked state (boolean) as input.
- Type: `function`
- Required: Yes

### showHint

If set to false the On/Off hint text will not appear alongside the toggle control. Default is true.

- Type: `Boolean`
- Required: No
3 changes: 2 additions & 1 deletion components/toggle-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class ToggleControl extends Component {
}

render() {
const { label, checked, help, instanceId } = this.props;
const { label, checked, help, instanceId, showHint } = this.props;
const id = `inspector-toggle-control-${ instanceId }`;

let describedBy;
Expand All @@ -45,6 +45,7 @@ class ToggleControl extends Component {
checked={ checked }
onChange={ this.onChange }
aria-describedby={ describedBy }
showHint={ showHint }
/>
</BaseControl>
);
Expand Down
19 changes: 8 additions & 11 deletions editor/components/post-comments/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,29 +7,26 @@ import { connect } from 'react-redux';
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { FormToggle, withInstanceId } from '@wordpress/components';
import { ToggleControl } from '@wordpress/components';

/**
* Internal Dependencies
*/
import { getEditedPostAttribute } from '../../store/selectors';
import { editPost } from '../../store/actions';

function PostComments( { commentStatus = 'open', instanceId, ...props } ) {
function PostComments( { commentStatus = 'open', ...props } ) {
const onToggleComments = () => props.editPost( { comment_status: commentStatus === 'open' ? 'closed' : 'open' } );

const commentsToggleId = 'allow-comments-toggle-' + instanceId;

return [
<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

label={ __( 'Allow Comments' ) }
checked={ commentStatus === 'open' }
onChange={ onToggleComments }
showHint={ false }
id={ commentsToggleId }
/>,
];
/>
);
}

export default connect(
Expand All @@ -41,5 +38,5 @@ export default connect(
{
editPost,
}
)( withInstanceId( PostComments ) );
)( PostComments );

12 changes: 5 additions & 7 deletions editor/components/post-pending-status/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { connect } from 'react-redux';
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { FormToggle, withInstanceId } from '@wordpress/components';
import { ToggleControl } from '@wordpress/components';
import { compose } from '@wordpress/element';

/**
Expand All @@ -17,18 +17,17 @@ import PostPendingStatusCheck from './check';
import { getEditedPostAttribute } from '../../store/selectors';
import { editPost } from '../../store/actions';

export function PostPendingStatus( { instanceId, status, onUpdateStatus } ) {
const pendingId = 'pending-toggle-' + instanceId;
export function PostPendingStatus( { status, onUpdateStatus } ) {
const togglePendingStatus = () => {
const updatedStatus = status === 'pending' ? 'draft' : 'pending';
onUpdateStatus( updatedStatus );
};

return (
<PostPendingStatusCheck>
<label htmlFor={ pendingId }>{ __( 'Pending Review' ) }</label>
<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.

This key is not necessary.

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

label={ __( 'Pending Review' ) }
checked={ status === 'pending' }
onChange={ togglePendingStatus }
showHint={ false }
Expand All @@ -50,5 +49,4 @@ const applyConnect = connect(

export default compose(
applyConnect,
withInstanceId
)( PostPendingStatus );
14 changes: 5 additions & 9 deletions editor/components/post-pingbacks/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,27 +7,24 @@ import { connect } from 'react-redux';
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { FormToggle, withInstanceId } from '@wordpress/components';
import { ToggleControl } from '@wordpress/components';

/**
* Internal Dependencies
*/
import { getEditedPostAttribute } from '../../store/selectors';
import { editPost } from '../../store/actions';

function PostPingbacks( { pingStatus = 'open', instanceId, ...props } ) {
function PostPingbacks( { pingStatus = 'open', ...props } ) {
const onTogglePingback = () => props.editPost( { ping_status: pingStatus === 'open' ? 'closed' : 'open' } );

const pingbacksToggleId = 'allow-pingbacks-toggle-' + instanceId;

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

label={ __( 'Allow Pingbacks & Trackbacks' ) }
checked={ pingStatus === 'open' }
onChange={ onTogglePingback }
showHint={ false }
id={ pingbacksToggleId }
/>,
];
}
Expand All @@ -41,5 +38,4 @@ export default connect(
{
editPost,
}
)( withInstanceId( PostPingbacks ) );

)( PostPingbacks );
12 changes: 4 additions & 8 deletions editor/components/post-sticky/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { connect } from 'react-redux';
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { FormToggle, withInstanceId } from '@wordpress/components';
import { ToggleControl } from '@wordpress/components';
import { compose } from '@wordpress/element';

/**
Expand All @@ -17,18 +17,15 @@ import { getEditedPostAttribute } from '../../store/selectors';
import { editPost } from '../../store/actions';
import PostStickyCheck from './check';

export function PostSticky( { onUpdateSticky, postSticky = false, instanceId } ) {
const stickyToggleId = 'post-sticky-toggle-' + instanceId;

export function PostSticky( { onUpdateSticky, postSticky = false } ) {
return (
<PostStickyCheck>
<label htmlFor={ stickyToggleId }>{ __( 'Stick to the Front Page' ) }</label>
<FormToggle
<ToggleControl
key="toggle"
label={ __( 'Stick to the Front Page' ) }
checked={ postSticky }
onChange={ () => onUpdateSticky( ! postSticky ) }
showHint={ false }
id={ stickyToggleId }
/>
</PostStickyCheck>
);
Expand All @@ -51,5 +48,4 @@ const applyConnect = connect(

export default compose( [
applyConnect,
withInstanceId,
] )( PostSticky );