feat(rome_js_analyzer): rule noAutofocus
#3333
Conversation
✅ Deploy Preview for rometools ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
alright, let me update this |
| } = node.as_fields(); | ||
|
|
||
| if name.ok()?.text().trim() == "input" { | ||
| let attribute = node.find_attribute_by_name("autoFocus").ok()??; |
There was a problem hiding this comment.
sry, I do not fully understand this, can you explain more?
|
I've updated the code based on the feedback. 🙏🏻 |
| fn diagnostic(_ctx: &RuleContext<Self>, attr: &Self::State) -> Option<RuleDiagnostic> { | ||
| Some(RuleDiagnostic::new( | ||
| rule_category!(), | ||
| attr.range(), |
There was a problem hiding this comment.
ahh, let me change it
|
If we only wanna test for fn run(ctx: &RuleContext<Self>) -> Self::Signals {
let node = ctx.query();
let name = node.name().ok()?.syntax().text_trimmed();
if name == "input" {
let attribute = node.find_attribute_by_name("autoFocus").ok()??;
is_value_undefined(&attribute).then_some(attribute)
} else {
None
}
}
fn is_value_undefined(attribute: &JsxAttribute) -> bool {
attribute
.initializer().and_then(|x| {
let name = x.value()
.ok()?
.as_jsx_expression_attribute_value()?
.expression()
.ok()?
.as_js_identifier_expression()?
.name()
.ok()?
.syntax().text_trimmed();
Some(name == "undefined")
}).unwrap_or(false)
}This correctly navigates the AST until finds an |
| ///``` | ||
| /// | ||
| /// ```jsx | ||
| /// <Input autoFocus={"false"} /> |
There was a problem hiding this comment.
If the only difference is the uppercase "I" I would put a comment around it.
| @@ -0,0 +1,5 @@ | |||
| <> | |||
| <input /> | |||
| <input autoFocus={undefined} /> | |||
There was a problem hiding this comment.
<input autoFocus={null} /> is valid or invalid?
There was a problem hiding this comment.
I believe that null props don't get rendered, but I might be wrong.
| <> | ||
| <input autoFocus /> | ||
| <input autoFocus="true" /> | ||
| <input autoFocus={"false"} /> |
There was a problem hiding this comment.
Would be interesting to also see
<input autoFocus={false} />
<input autoFocus={a} />| /// <input autoFocus={undefined} /> | ||
| ///``` | ||
| /// | ||
| /// ```jsx | ||
| /// <Input autoFocus={"false"} /> |
There was a problem hiding this comment.
I think these cases should fail. Check the original rule: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/main/docs/rules/no-autofocus.md#fail
There was a problem hiding this comment.
Alright, because I was following this docs. If we want to follow eslint-plugin-jsx-a11y, then it should be any element with autoFocus attribute should be avoided. https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/f6ba03cd5c206e4ca04068253ab79ad32eb929ce/src/rules/no-autofocus.js
There was a problem hiding this comment.
Yeah apologies for that. Probably that rule was buggy to begin with. Yeah it's best to follow the a11y plugin, it's been around longer and they catch more cases.
There was a problem hiding this comment.
No worries! I will change the implementation to follow a11y plugin.
There was a problem hiding this comment.
Hey, eslint-plugin-jsx-a11y provide a config ignoreNonDOM to ignore user created component autoFocus attribute, e.g. <MyComponent autoFocus={true} />. I wonder how we should do that. Should we include the user-created components or just ignore them or provides an additional option to let the user to config?
There was a problem hiding this comment.
FYI, this is how to config to ignore user-created components in eslint
"rules": {
"jsx-a11y/no-autofocus": [ 2, {
"ignoreNonDOM": true
}]
}There was a problem hiding this comment.
As for now, we can't configure rules with options. I think we should always ignore user-created components and just inspect normal elements.
|
Hi, I've re-implemented the PS: I'm using |
That's the right approach! 💯 |
Summary
Closes #3297
Test Plan
Added test cases to test the situations according to this docs.