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(eslint-plugin-template): [accessibility-valid-aria] add suggestion #489

Conversation

rafaelss95
Copy link
Member

No description provided.

@nx-cloud
Copy link

nx-cloud bot commented May 19, 2021

Nx Cloud Report

CI ran the following commands for commit e8dbcb2. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=build --all --parallel
#000000 nx run-many --target=check-configs --all --parallel
#000000 nx run-many --target=test --all --parallel
#000000 nx run-many --target=typecheck --all --parallel

Sent with 💌 from NxCloud.

@@ -47,7 +47,7 @@ ruleTester.run(RULE_NAME, rule, {
<div aria-roledescriptio="text">Text</div>
Copy link
Member

Choose a reason for hiding this comment

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

Hmm conceptually is the "fix" here really to remove what the user has written here?

Let's imagine this unit test is a real scenario, they clearly tried to add a real attribute and just made a typo. Then they run linting with --fix and instead of having chance to review their mistake and make their markup more accessible for users, they just have an unannotated div...

Maybe we have to go for suggestion only on this rule for that reason?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you have a point. In this case, let's wait for #491 that offers the support for multiple suggestions in convertAnnotatedSourceToFailureCase.

Copy link
Member Author

Choose a reason for hiding this comment

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

@JamesHenry I just noticed that in codelyzer#templateAccessibilityValidAriaRule.ts#L35 we suggest the closest aria attribute possible, instead of removing it, based on damerau-levenshtein.

So my question is: should we add this lib to mimic this behavior or is it fine as is?

@rafaelss95 rafaelss95 force-pushed the feat/accessibility-valid-aria-fixer branch from e6f8091 to e8dbcb2 Compare May 22, 2021 17:49
@rafaelss95 rafaelss95 changed the title feat(eslint-plugin-template): [accessibility-valid-aria] add fixer feat(eslint-plugin-template): [accessibility-valid-aria] add suggestion May 22, 2021
@rafaelss95 rafaelss95 marked this pull request as ready for review May 22, 2021 17:49
@JamesHenry JamesHenry merged commit 678e1b5 into angular-eslint:master May 22, 2021
@rafaelss95 rafaelss95 deleted the feat/accessibility-valid-aria-fixer branch May 22, 2021 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants