feat(label-has-associated-control): add option for enforcing label's htmlFor matches control's id #1042
Conversation
This file contains hidden or 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 & { |
Contributor Author
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 & { |
Contributor Author
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 Contributor Author
| @ljharb I realized this is still pending review. Is this feature still something you want to move forward with? |
Member
| Yes, thanks for your patience. |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
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