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

Add inspector controls #1278

Merged
merged 6 commits into from
Jun 23, 2017

Conversation

paulwilde
Copy link
Contributor

This is a rebased version of #1202 whilst also including #1198.

@aduth
Copy link
Member

aduth commented Jun 20, 2017

There are some failing tests here due to missing key prop and trailing spaces:

https://travis-ci.org/WordPress/gutenberg/jobs/244678746

As a quality-of-life tip, when rebasing, you needn't create a separate pull request, but rather can force push atop your existing branch (e.g. git push -f origin add/inspector-controls)


return (
<BaseControl label={ heading } id={ id }>
<label className="blocks-checkbox-control__label" htmlFor={ id }>
Copy link
Member

Choose a reason for hiding this comment

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

Missed this before. We shouldn't combine approaches of implicit and explicit labelling, between htmlFor and nesting input as a child of the label. From previous conversations with @afercia , htmlFor should be our preferred approach as it is better supported by screen readers.

#527 (comment)

@aduth
Copy link
Member

aduth commented Jun 21, 2017

This looks good to me 👍

* WordPress dependencies
*/
import { withInstanceId } from 'components';
import Toggle from 'components/form-toggle';
Copy link
Member

Choose a reason for hiding this comment

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

Minor: We should import from the top-level, i.e. import { withInstanceId, Toggle } from 'components';

* WordPress dependencies
*/
import { withInstanceId } from 'components';
import { isEmpty } from 'lodash';
Copy link
Member

Choose a reason for hiding this comment

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

lodash is an external dependency, not a WordPress dependency.

* WordPress dependencies
*/
import { withInstanceId } from 'components';
import { isEmpty } from 'lodash';
Copy link
Member

Choose a reason for hiding this comment

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

Same note about Lodash from previous.

/**
* WordPress dependencies
*/
import { withInstanceId, Toggle } from 'components';
Copy link
Member

@aduth aduth Jun 21, 2017

Choose a reason for hiding this comment

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

Oof, while this is preferred, we're also not currently exporting Toggle from components/index.js as we should be. Causes an error when selecting the text block.

@youknowriad
Copy link
Contributor

I took the liberty to update this to fix the ToggleControl issue. I'm going to merge this to unblock #1191

Thanks awesome work here.

@youknowriad youknowriad merged commit fb0e60f into WordPress:master Jun 23, 2017
@paulwilde paulwilde deleted the add/inspector-controls-2 branch June 23, 2017 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants