This repository has been archived by the owner on Jul 15, 2023. It is now read-only.
exclude specific input types from react-a11y-input-elements rule #754
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
JoshuaKGoldberg
suggested changes
Jan 1, 2019
JoshuaKGoldberg
suggested changes
Jan 1, 2019
lizzzp1
commented
Jan 1, 2019
| if (isEmpty(attributes.value) && isEmpty(attributes.placeholder)) { | ||
| const isExcludedInputTypeValueEmpty = isEmpty(attributes.value) && isExcludedInputType(node); | ||
| const isPlaceholderEmpty = isEmpty(attributes.placeholder) && !isExcludedInputType(node); | ||
| if ((isEmpty(attributes.value) && isPlaceholderEmpty) || isExcludedInputTypeValueEmpty) { | ||
| ctx.addFailureAt(node.getStart(), node.getWidth(), MISSING_PLACEHOLDER_INPUT_FAILURE_STRING); |
There was a problem hiding this comment.
😬 might this be simplified?
There was a problem hiding this comment.
A couple things to try:
- Pass
attributesintoisExcludedInputType.typeso you don't have to callgetJsxAttributesFromJsxElementeach time - Only make one call to
isExcludedInputType(node)and store that in a variable, so you don't need to call it twice.
...but the test cases look good, build passes, and this is still a pretty simple rule; I'm fine with merging this in as is & filing a followup issue to look into cleaning it up.
I'll wait for direction from you if you want to tackle it now - totally up to you! 😊
There was a problem hiding this comment.
Good call! Dried it up ⬇️
JoshuaKGoldberg
approved these changes
Jan 1, 2019
lizzzp1
commented
Jan 2, 2019
| @@ -46,10 +46,10 @@ export class Rule extends Lint.Rules.AbstractRule { | |||
| } | |||
| } | |||
|
|
|||
| function isExcludedInputType(node: ts.JsxSelfClosingElement): boolean { | |||
| function isExcludedInputType(node: ts.JsxSelfClosingElement, attributes: { [propName: string]: ts.JsxAttribute }): boolean { | |||
| for (const attribute of node.attributes.properties) { | |||
There was a problem hiding this comment.
got this type from below. Let me know if this looks right to you.
JoshuaKGoldberg
approved these changes
Jan 2, 2019
There was a problem hiding this comment.
LGTM, thanks @lizzzp1! Exciting how quick this bug was fixed 🙌
apawast
pushed a commit
to lupine86/tslint-microsoft-contrib
that referenced
this pull request
Feb 26, 2019
…rosoft#754) * exclude specific input types from react-a11y-input-elements rule * feedback - use for of, tsutils for type checks * feedback - add tests for non type attributes with excluded values, clean up boolean * dry things up
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
PR checklist
react-a11y-input-elementsincorrectly requiring placeholder for radio, checkbox, and file inputs #749Overview of change:
Excludes the 3 input types listed in issue from requiring a placeholder.
Is there anything you'd like reviewers to focus on?