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

Jump to bottom

Add: ESLint rule that forbids a BaseControl that includes label prop but omits id #14151

Merged
merged 1 commit into from
Apr 24, 2019
Merged

Add: ESLint rule that forbids a BaseControl that includes label prop but omits id #14151

merged 1 commit into from
Apr 24, 2019

Conversation

jorgefilipecosta
Copy link
Member

Fixes: #6989

Follows a suggestion by @aduth and creates an eslint rule that forbids a BaseControl that includes label prop but omits id.

Currently, CI is red because some code needs an update. I will submit right away a PR that fixes issue #13894 and makes the code compliant with our lint rule.

For now, I added some ignores, each case has it its complexities. For each, we need to decide if the usage of BaseControl makes sense, if an id should be provided or if the label needs to be removed a new child used instead.
I will submit a PR per case so we can discuss each case separately and make sure we don't create accessibility regressions.

@jorgefilipecosta jorgefilipecosta force-pushed the add/add-Eslint-rule-to-avoid-BaseControl-component-with-label-without-an-id branch from 2143bbd to 1f91a94 Compare February 27, 2019 19:23
@jorgefilipecosta jorgefilipecosta mentioned this pull request Feb 27, 2019
Merged
@jorgefilipecosta jorgefilipecosta added [Tool] ESLint plugin /packages/eslint-plugin [Type] Code Quality Issues or PRs that relate to code quality labels Feb 27, 2019
@jorgefilipecosta jorgefilipecosta mentioned this pull request Feb 28, 2019
Merged
@jorgefilipecosta jorgefilipecosta force-pushed the add/add-Eslint-rule-to-avoid-BaseControl-component-with-label-without-an-id branch from 1f91a94 to 4d0157c Compare March 1, 2019 18:11
@jorgefilipecosta jorgefilipecosta marked this pull request as ready for review March 1, 2019 18:11
@jorgefilipecosta jorgefilipecosta force-pushed the add/add-Eslint-rule-to-avoid-BaseControl-component-with-label-without-an-id branch from 4d0157c to a9824d1 Compare March 1, 2019 18:12
aduth
aduth reviewed Mar 1, 2019
View reviewed changes
packages/eslint-plugin/docs/rules/no-base-control-with-label-without-id.md
```jsx
<BaseControl
label="ok"
id="my-id"
Copy link
Member

Choose a reason for hiding this comment

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

Even if this is technically "correct" by the condition of the lint rule itself, it's still not a good example in that the id should direct to an input that the BaseControl wraps. Rather than <b>Child</b>, I'd prefer to see something like <input id="my-id" />

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @aduth thank you for the review 👍 Your feedback was addressed.

@aduth aduth mentioned this pull request Mar 1, 2019
Closed
@jorgefilipecosta jorgefilipecosta added the [Status] In Progress Tracking issues with work in progress label Mar 1, 2019
@jorgefilipecosta jorgefilipecosta force-pushed the add/add-Eslint-rule-to-avoid-BaseControl-component-with-label-without-an-id branch from a9824d1 to d282f2c Compare March 4, 2019 09:53
@jorgefilipecosta jorgefilipecosta removed the [Status] In Progress Tracking issues with work in progress label Mar 5, 2019
aduth
aduth reviewed Mar 11, 2019
View reviewed changes
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

I think it may be good to wait to see how #14179 shakes out before moving forward on this.

I had a review comment saved, so submitting to have it published.

packages/block-library/src/video/edit.js Outdated
ref={ this.posterImageButton }
>
{ ! this.props.attributes.poster ? __( 'Select Poster Image' ) : __( 'Replace image' ) }
{ (
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, would it not have worked to just do something like?

{ /* TODO: ... */ }
{ /* eslint-disable-next-line @wordpress/no-base-control-with-label-without-id */ }

(avoid the extra nesting level)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it would work, I simply used another nesting level because it was the idea that occurred to me.

jorgefilipecosta added a commit that referenced this pull request Mar 28, 2019
@jorgefilipecosta
Code quality: Remove unnecessary BaseControl usage from video block (#…
bde17b6 
…14179)

## Description
This PR applies a simple change to remove unnecessary BaseControl usage in the video block.
It makes the video block code compliant with the lint rule being added on #14151.

I tried a different approach use the BaseControl as a label for the button being rendered inside it, but in my tests with a screen reader the button text stops being used and BaseControl label starts getting used, in this case, this change does not make sense, the button text should be used.

## How has this been tested?
I checked that the Poster Image buttons still work.
I checked that no visual changes happen.
@jorgefilipecosta jorgefilipecosta force-pushed the add/add-Eslint-rule-to-avoid-BaseControl-component-with-label-without-an-id branch from d282f2c to af8a8a1 Compare March 28, 2019 16:15
@jorgefilipecosta jorgefilipecosta force-pushed the add/add-Eslint-rule-to-avoid-BaseControl-component-with-label-without-an-id branch from af8a8a1 to 72c2dc4 Compare March 28, 2019 19:53
@jorgefilipecosta
Copy link
Member Author

This PR was updated and dependencies were merged, it should be ready for another look.

@jorgefilipecosta jorgefilipecosta force-pushed the add/add-Eslint-rule-to-avoid-BaseControl-component-with-label-without-an-id branch from 72c2dc4 to 3bc3875 Compare April 12, 2019 13:26
aduth
aduth approved these changes Apr 24, 2019
View reviewed changes
Copy link
Member

@aduth aduth left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM 👍 I also tested after rebasing the branch locally to make sure there were no recent additions which would violate this rule (and thus would cause a build failure if merged without fix). There were none.

Per a recent discussion in Core JS meeting about which rules to consider "Recommended", I contemplated whether it would be a good idea to make it recommended here. While we might consider improving this in the future to guarantee that "BaseControl" is in-fact the one provided by @wordpress/components, I think it's fair to say that if someone is referencing this component, it would be recommended to abide by this rule.

@aduth aduth merged commit 53b012a into master Apr 24, 2019
@aduth aduth deleted the add/add-Eslint-rule-to-avoid-BaseControl-component-with-label-without-an-id branch April 24, 2019 13:23
This was referenced Apr 24, 2019
Closed
Merged
Merged
Merged
@youknowriad youknowriad added this to the 5.6 (Gutenberg) milestone May 13, 2019
This was referenced Sep 12, 2024
Open
Open
Open
@AKJUS AKJUS mentioned this pull request Sep 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduth aduth aduth approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten

@gziolo gziolo Awaiting requested review from gziolo

@nerrad nerrad Awaiting requested review from nerrad

@noisysocks noisysocks Awaiting requested review from noisysocks

@oandregal oandregal Awaiting requested review from oandregal

@notnownikki notnownikki Awaiting requested review from notnownikki

@ntwb ntwb Awaiting requested review from ntwb

@Soean Soean Awaiting requested review from Soean

@talldan talldan Awaiting requested review from talldan

@youknowriad youknowriad Awaiting requested review from youknowriad

@ellatrix ellatrix Awaiting requested review from ellatrix

Assignees
No one assigned
Labels
[Tool] ESLint plugin /packages/eslint-plugin [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Milestone
5.6 (Gutenberg)
Development

Successfully merging this pull request may close these issues.

BaseControl should require an id or create one if missing
3 participants
@jorgefilipecosta @aduth @youknowriad