feat(label-has-associated-control): add option for enforcing label's htmlFor matches control's id #1042
+689
−63
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
michaelfaith
changed the title
michaelfaith
force-pushed
the
feat/label-has-associated-control-option
branch
7 times, most recently
from
c5c7649 to
789d368
Compare
…htmlFor matches control's id This change adds an option to the `label-has-associated-control` rule, enforcing that the label's htmlFor attribute matches the associated control's id attribute. Previously, the only validation done on htmlFor was that it was on the label component and had text. There was no attempt to cross-check that value against any attribute on the associated control. Not, when the option is enabled, cases where they don't match will report. I also took the opportunity to update the error messages so that each assert type gets an error message with verbiage specific to the assertion. (not sure if this should be called out as a separate feature in the changelog?). Note: the current implementation only checks the first instance it finds of child component that matches each control component type. It assumes that there won't be any acceptable cases where a label would have multiple inputs nested beneath it. Let me know if that assumption doesn't hold. Closes:
michaelfaith
force-pushed
the
feat/label-has-associated-control-option
branch
from
789d368 to
0f1845a
Compare
michaelfaith
commented
Dec 25, 2024
| type: 'JSXClosingFragment', | ||
| }; | ||
|
|
||
| export type JSXFragment = JSXElement & { |
There was a problem hiding this comment.
It would have been better to use Omit here to remove openingElement and closingElement from JSXElement, but this version of flow doesn't have Omit (that was introduced in 0.211.0). I'd be up for upgrading the version of flow in a separate change if you'd like?
michaelfaith
commented
Dec 25, 2024
| type: 'JSXClosingFragment', | ||
| }; | ||
|
|
||
| export type JSXFragment = JSXElement & { |
There was a problem hiding this comment.
The ast-types-flow package doesn't include a type for JSXFragment
michaelfaith
changed the title
label-has-associated-control): add option for enforcing label's htmlFor matches control's id
michaelfaith
changed the title
label-has-associated-control): add option for enforcing label's htmlFor matches control's id
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change adds an option to the
label-has-associated-controlrule, enforcing that the label's htmlFor attribute matches the associated control's id attribute. Previously, the only validation done on htmlFor was that it was on the label component and had text. There was no attempt to cross-check that value against any attribute on the associated control. Not, when the option is enabled, cases where they don't match will report.I also took the opportunity to update the error messages so that each assert type gets an error message with verbiage specific to the assertion. (not sure if this should be called out as a separate feature in the changelog?).
Note: the current implementation only checks the first instance it finds of a child component that matches each control component type. It assumes that there won't be any acceptable cases where a label would have multiple inputs nested beneath it. Let me know if that assumption doesn't hold and i can refine that logic more.
Closes: #956