-
- Notifications
You must be signed in to change notification settings - Fork 2.7k
[New] add no-invalid-html-attribute rule #2863
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
Conversation
Co-authored-by: Sebastian Malton <sebastian@malton.name> Co-authored-by: Jordan Harband <ljharb@gmail.com>
ljharb left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plus all of @golopot's comments.
d18a7fe to f0f654a Compare
ljharb left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there other HTML element attributes that this type of rule might work on?
It might be nicer to make this rule a generic no-invalid-html-attribute rule (one that also works on React.createElement, and thus isn't jsx-specific), with an option for a list of attributes to check (that would default to ['rel']). That way, we could add support for a new attribute in a semver-minor without needing to add an entirely new rule.
Thoughts?
| @ljharb Good idea about making this a more generic rule, though I don't know if I want to make it working with |
| @ljharb I have changed the name of the rule and added the option as you said. While reworking the internals a bit (though I haven't tested the code with non-rel items) |
ljharb left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is looking much better
| I know that there are some failing tests for some older versions of eslint. Will look at them soon. |
| No rush, please mark as ready for review once tests are passing. |
ljharb left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks great! (sorry for the review delay)
This rule isn't named "jsx-", but it doesn't check React.createElement. I think it probably should, though. Is that something you'd mind adding?
| @ljharb I have added checking |
ljharb left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there other attributes besides "rel" we can add here?
Codecov Report
@@ Coverage Diff @@ ## master #2863 +/- ## ========================================== - Coverage 97.22% 97.15% -0.07% ========================================== Files 110 111 +1 Lines 7312 7428 +116 Branches 2669 2701 +32 ========================================== + Hits 7109 7217 +108 - Misses 203 211 +8
Continue to review full report at Codecov.
|
| Maybe https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/autocomplete but I think that could easily be done in a separate PR. |
| Rebased again; I tweaked the test. A few tests are still failing. @Nokel81? |
| Well since you just changed the logic or tests (cannot tell). I was whitelisting elements whereas I think you want me to blacklist html elements that aren't on the list. |
| All I did is move the two "Foo" tests from invalid to valid. |
| I'm fine with an inclusion-list-based approach, but it should ignore custom elements entirely. |
e33e82f to 4d05d57 Compare 4d05d57 to 11764b5 Compare | @ljharb Sorry it took so long, this PR has been updated to ignore non-HTML tags. |
no-invalid-html-attribute rule | @Nokel81 i enabled all the test cases and used the messageId pattern and rebased. |
This rule (and fixer) checks the "rel" value to make sure that it correctly relates to the tag that the "rel" property is on.