Skip to content
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

feat(biome_js_analyze): add two recommended rules in eslint-plugin-jsx-a11y #517

Merged
merged 14 commits into from
Oct 15, 2023

Conversation

nissy-dev
Copy link
Contributor

@nissy-dev nissy-dev temporarily deployed to Website deployment October 12, 2023 15:00 — with GitHub Actions Inactive
@github-actions github-actions bot added A-Project Area: project A-Linter Area: linter A-Website Area: website L-JavaScript Language: JavaScript and super languages A-Diagnostic Area: diagnostocis A-Changelog Area: changelog labels Oct 12, 2023
@nissy-dev nissy-dev temporarily deployed to Website deployment October 13, 2023 01:07 — with GitHub Actions Inactive
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments. I am happy to merge once they are addressed.

Comment on lines 86 to 96
RuleDiagnostic::new(
rule_category!(),
node.range(),
markup! {
"Interactive elements should not be assigned non-interactive roles."
},
)
.note(markup! {
"Wrap your interactive element in a <div> with the desired role or put the content inside your interactive element."
}),
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a note that tells the user why "Interactive elements should not be assigned non-interactive roles."

Comment on lines 79 to 88
RuleDiagnostic::new(
rule_category!(),
node.range(),
markup! {
"Enforce elements with aria-activedescendant are tabbable."
},
)
.note(markup! {
"Add the tabIndex attribute to the element with a value greater than or equal to -1."
}),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a note telling the user why they should enforce a tab index.

…tivedescendant_tabindex.rs

Co-authored-by: Emanuele Stoppa <[email protected]>
@nissy-dev nissy-dev temporarily deployed to Website deployment October 14, 2023 11:28 — with GitHub Actions Inactive
nissy-dev and others added 2 commits October 14, 2023 20:28
…tivedescendant_tabindex.rs

Co-authored-by: Emanuele Stoppa <[email protected]>
…ive_element_to_noninteractive_role.rs

Co-authored-by: Emanuele Stoppa <[email protected]>
@nissy-dev nissy-dev temporarily deployed to Website deployment October 14, 2023 11:28 — with GitHub Actions Inactive
Copy link
Member

@Conaclos Conaclos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we could use shorter names for these rules?

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
nissy-dev and others added 3 commits October 14, 2023 23:36
Co-authored-by: Victorien Elvinger <[email protected]>
Co-authored-by: Victorien Elvinger <[email protected]>
@nissy-dev nissy-dev temporarily deployed to Website deployment October 14, 2023 14:46 — with GitHub Actions Inactive
@nissy-dev
Copy link
Contributor Author

Do you think we could use shorter names for these rules?

I think it would be better if possible, but I can't come up with a specific name.

@Conaclos
Copy link
Member

Conaclos commented Oct 14, 2023

Do you think we could use shorter names for these rules?

I think it would be better if possible, but I can't come up with a specific name.

Yes. The only simplification I see is removing With from the second rule name. This is not worth the effort.

By the way, I think we already implemented the first rule...

@nissy-dev nissy-dev temporarily deployed to Website deployment October 15, 2023 03:53 — with GitHub Actions Inactive
@nissy-dev
Copy link
Contributor Author

By the way, I think we already implemented the first rule...

Yeah... These rules are very confusing...

@nissy-dev nissy-dev merged commit 57a3d9d into main Oct 15, 2023
17 checks passed
@nissy-dev nissy-dev deleted the new-rule branch October 15, 2023 04:45
@nissy-dev
Copy link
Contributor Author

I updated diagnostic notes, so I merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Diagnostic Area: diagnostocis A-Linter Area: linter A-Project Area: project A-Website Area: website L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants