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

Control has associated label #515

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@jessebeach
Copy link
Collaborator

jessebeach commented Dec 27, 2018

This rule is the inverse of label-has-associated-control.

Check the Markdown file for details of the rule.

It's intended to catch unlabelled buttons, checkboxes, radios, menuitems, options, etc. I did a bunch of QA work to finagle the configs so that the output is actionable.

The danger here is that we encourage developers to put labels on things like containers, which really shouldn't be labelled. The exceptions to that rule are dialogs and alerts, but I opened a new issue to write a rule for those: #516

There really isn't a difference between the behavior of recommended and strict, so I made the configs the same for both.

@jessebeach jessebeach force-pushed the jessebeach:control-has-associated-label branch from defcc00 to 3544dfd Dec 27, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 27, 2018

Coverage Status

Coverage increased (+0.004%) to 99.445% when pulling 3544dfd on jessebeach:control-has-associated-label into e53906d on evcohen:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 27, 2018

Coverage Status

Coverage increased (+0.01%) to 99.457% when pulling ca14de5 on jessebeach:control-has-associated-label into fff6a4b on evcohen:master.

Show resolved Hide resolved __tests__/src/rules/control-has-associated-label-test.js Outdated
Show resolved Hide resolved src/index.js Outdated

@ljharb ljharb added the new-rule label Dec 27, 2018

@jessebeach jessebeach force-pushed the jessebeach:control-has-associated-label branch 4 times, most recently from 98c8398 to 4833376 Dec 27, 2018

@jessebeach

This comment has been minimized.

Copy link
Collaborator

jessebeach commented Dec 28, 2018

Should we enforce a label on canvas, audio and video tags?

@jessebeach

This comment has been minimized.

Copy link
Collaborator

jessebeach commented Dec 28, 2018

I'm testing the rule on the FB codebase right now and fine-tuning it. It's over-capturing a little.

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Dec 28, 2018

It seems like maybe the default should be to enforce a label on everything, but maybe have an option to ignore input types that aren't that important to label, like canvas/audio/video?

@jessebeach jessebeach force-pushed the jessebeach:control-has-associated-label branch 2 times, most recently from 7d3bf8a to cf98d68 Dec 28, 2018

@jessebeach

This comment has been minimized.

Copy link
Collaborator

jessebeach commented Dec 28, 2018

The results from my internal tests look good. About 250 errors over ~70K files. Keep in mind that we've done a TON of work to ferret out unlabelled elements. These represent the ones that we had no signal for.

I just need to fill out the control-has-associated-label.md file and this rule should be good to go.

@jessebeach

This comment has been minimized.

Copy link
Collaborator

jessebeach commented Dec 29, 2018

I'm finding lots of juicy examples where all the rules in the plugin have been followed except that the element doesn't have a label. This is the last step!

@jessebeach jessebeach force-pushed the jessebeach:control-has-associated-label branch from cf98d68 to 38f9fbc Dec 29, 2018

@jessebeach jessebeach force-pushed the jessebeach:control-has-associated-label branch from 38f9fbc to a3fa108 Dec 29, 2018

@jessebeach jessebeach requested review from evcohen and lencioni Dec 29, 2018

jessebeach added some commits Dec 30, 2018

@jessebeach

This comment has been minimized.

Copy link
Collaborator

jessebeach commented Jan 5, 2019

@ljharb @evcohen thoughts on merging this new rule in? I ran it over the FB codebase and the results are great. Fixable and accurate.

@ljharb

ljharb approved these changes Jan 6, 2019

Copy link
Collaborator

ljharb left a comment

This seems great. It'd be ideal to rebase this first, so there's no merge commits, but otherwise let's merge, and cut a minor release :-D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment