Conversation
jorgefilipecosta
force-pushed
the
add/add-Eslint-rule-to-avoid-BaseControl-component-with-label-without-an-id
branch
from
2143bbd to
1f91a94
Compare
jorgefilipecosta
added
[Tool] ESLint plugin
jorgefilipecosta
force-pushed
the
add/add-Eslint-rule-to-avoid-BaseControl-component-with-label-without-an-id
branch
from
1f91a94 to
4d0157c
Compare
jorgefilipecosta
requested review from
Soean,
aduth,
ajitbohra,
chrisvanpatten,
gziolo,
nerrad,
noisysocks,
notnownikki,
ntwb,
oandregal,
talldan and
youknowriad
as code owners
jorgefilipecosta
force-pushed
the
add/add-Eslint-rule-to-avoid-BaseControl-component-with-label-without-an-id
branch
from
4d0157c to
a9824d1
Compare
| ```jsx | ||
| <BaseControl | ||
| label="ok" | ||
| id="my-id" |
There was a problem hiding this comment.
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" />
There was a problem hiding this comment.
Hi @aduth thank you for the review 👍 Your feedback was addressed.
jorgefilipecosta
added
the
[Status] In Progress
jorgefilipecosta
force-pushed
the
add/add-Eslint-rule-to-avoid-BaseControl-component-with-label-without-an-id
branch
from
a9824d1 to
d282f2c
Compare
jorgefilipecosta
removed
the
[Status] In Progress
| ref={ this.posterImageButton } | ||
| > | ||
| { ! this.props.attributes.poster ? __( 'Select Poster Image' ) : __( 'Replace image' ) } | ||
| { ( |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
I think it would work, I simply used another nesting level because it was the idea that occurred to me.
…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
force-pushed
the
add/add-Eslint-rule-to-avoid-BaseControl-component-with-label-without-an-id
branch
from
d282f2c to
af8a8a1
Compare
jorgefilipecosta
force-pushed
the
add/add-Eslint-rule-to-avoid-BaseControl-component-with-label-without-an-id
branch
from
af8a8a1 to
72c2dc4
Compare
|
This PR was updated and dependencies were merged, it should be ready for another look. |
jorgefilipecosta
force-pushed
the
add/add-Eslint-rule-to-avoid-BaseControl-component-with-label-without-an-id
branch
from
72c2dc4 to
3bc3875
Compare
There was a problem hiding this comment.
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
deleted the
add/add-Eslint-rule-to-avoid-BaseControl-component-with-label-without-an-id
branch
3 participants
Fixes: #6989
Follows a suggestion by @aduth and creates an eslint rule that forbids a
BaseControlthat includeslabelprop but omitsid.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.