-
Notifications
You must be signed in to change notification settings - Fork 659
feat(rome_js_analyze): useHeadingContent
#4218
Conversation
✅ Deploy Preview for docs-rometools canceled.Built without sensitive environment variables
|
crates/rome_js_analyze/src/analyzers/nursery/use_heading_content.rs
Outdated
Show resolved
Hide resolved
I was wondering about these test cases: valid: invalid:
We have |
Thank you for your review! I try to modify codes to catch more invalid cases which you or eslint-plugin-jsx-a11y mentions 👍 |
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.
I think it's worth adding some other test cases.
}) | ||
.count() | ||
== 0 | ||
&& !node.has_dangerously_set_inner_html_attribute()? |
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.
We can move the check has_dangerously_set_inner_html_attribute
up, next to if node.is_heading_element()?
.filter(|child_node| { | ||
is_accessible_to_screen_reader(child_node) == Some(true) | ||
}) | ||
.count() | ||
== 0 |
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.
I think it's more readable to use the function any
instead of filter().count()
.
== 0 | ||
&& !node.has_dangerously_set_inner_html_attribute()? | ||
{ | ||
return Some(element.syntax().text_range()); |
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.
There's no need to return anything here. We already have element
, which is the Query
. The rule can just return Some(())
None | ||
} | ||
|
||
fn diagnostic(_: &RuleContext<Self>, reference: &Self::State) -> Option<RuleDiagnostic> { |
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.
As per the previous comment, you can use the context to retrieve the range:
let node = ctx.query();
let range = node.syntax().text_trimmed_range();
You have to use text_trimmed_range
@@ -0,0 +1,116 @@ | |||
use rome_js_syntax::{ |
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.
Nit: we already have a utils.rs
file with a utils/
folder. Having another utils*
file seems a bit redundant. My suggestions:
- having just a
aria.rs
file - adding these two functions inside the
utils.rs
file
} | ||
|
||
/// Check if the `aria-hidden` attribute is present or the value is true. | ||
pub(crate) fn is_aria_hidden_truthy(aria_hidden_attribute: &JsxAttribute) -> Option<bool> { |
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.
I believe this function doesn't need to be pub(crate)
<h1> | ||
<div aria-hidden /> | ||
</h1> | ||
<h1></h1> |
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.
Some invalid case we can add:
<>
<h1 aria-hidden="true">content</h1>
<h1 aria-hidden="true" dangerouslySetInnerHTML={{ __html: "heading" }} />
</>
Thank you for your reviews! I refactor codes and pass more tests. |
@nissy-dev Thank you! What do you think? |
@denbezrukov |
I merged #4342, so I will try to reimplement in a few days. |
@nissy-dev I think we can rebase the PR, fix the conflicts and we can merge it! |
I recreate PR in #4423 |
Summary
Fix #3943
Test Plan
I added snapshot tests
Documentation