chore(grants): add eslint-plugin-vuejs-accessibility and disable errors flagged #3634
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
dependencies
QA Summary
Test CoverageCoverage report for `packages/client`
Coverage report for `packages/server`
|
Terraform Summary
Hint: If "Terraform Format & Style" failed, run OutputValidation OutputPlan Summary
Pusher: @TylerHendrickson, Action: |
github-actions
bot
added
the
documentation
lsr-explore
force-pushed
the
chore/eslint-plugin-accessibility
branch
from
00897f2 to
8f4f747
Compare
lsr-explore
force-pushed
the
chore/eslint-plugin-accessibility
branch
2 times, most recently
from
0014b5b to
4713d4c
Compare
lsr-explore
force-pushed
the
chore/eslint-plugin-accessibility
branch
from
4713d4c to
a0ad3a8
Compare
There was a problem hiding this comment.
@lsr-explore Adding a few comments/considerations to the discussion. Thanks for putting this together!
|
|
||
| ### Negative Consequences <!-- optional --> | ||
|
|
||
| - Some devs may not know how to fix accessibility issues in Vue.js applications, and this may cause some friction in development or result in a number of rules being disabled. |
There was a problem hiding this comment.
I think it's likely that we could encounter this from time to time, although IMO that's certainly not a deal-breaker. That said, I'm wondering if we might be able to mitigate issues for newcomers by providing documentation – do you think there are any useful resource(s) that would be worth linking to from our docs/ directory, as a starting point?
There was a problem hiding this comment.
If this was adopted, I would write up some introductory documentation. The package has some documentation, but their solution isn't always the most optimal for accessibility.
https://vue-a11y.github.io/eslint-plugin-vuejs-accessibility/rules/click-events-have-key-events.html
There is a ton of resources - will need to look for something that would be not overwhelming.
I like your idea of creating issues if items need to be temporarily disabled, and we can use a common tag, so devs can see examples from the code.
I can also put together an introduction to Accessibility to be reviewed during onboarding.
There was a problem hiding this comment.
That would be excellent – thank you!
|
|
||
| [Changes are in this PR with the Decision Record - #3634](https://github.com/usdigitalresponse/usdr-gost/pull/3634/files) | ||
|
|
||
| I can separate out the changes into separate PR if needed. |
There was a problem hiding this comment.
Assuming we end up adopting at least one of the recommended linters, I don't think it should be necessary to separate the code changes from this PR into a separate PR. However, I do think that we should make a plan to address the newly-introduced eslint-disable directives, which might entail having individual issues (either per-file, or else something more aligned with perceived level of effort) along with a parent issue we can use to track burn-down progress.
There was a problem hiding this comment.
Sounds good. If this is adopted, I plan to work on addressing the issues. Before merging, I'll create tech debt tickets and add them to the disable lines. I agree with your suggestion of one ticket per page.
One of the findings - autofocus would need to be reviewed w/ UX as it would change the behavior of the application. I don't know if this behavior was added by devs or specified by UX.
Accessibility concerns
Automatically focusing a form control can confuse visually-impaired people using screen-reading technology and people with cognitive impairments. When autofocus is assigned, screen-readers "teleport" their user to the form control without warning them beforehand.Use careful consideration for accessibility when applying the autofocus attribute. Automatically focusing on a control can cause the page to scroll on load. The focus can also cause dynamic keyboards to display on some touch devices. While a screen reader will announce the label of the form control receiving focus, the screen reader will not announce anything before the label, and the sighted user on a small device will equally miss the context created by the preceding content.
label has for - I thought was an easy coding fix, but I ran into an conflict with the vue linter not allowing self closing elements - need to look into it a bit more
Key events and static interactions - This is generally fixed by using a semantic element - button, or link, but then the styles may need to be adjusted to match the current look and feel.
TylerHendrickson
approved these changes
Oct 21, 2024
There was a problem hiding this comment.
@lsr-explore I think this proposal is in a good place and we haven't received any other concerns/pushback from the rest of the team, so I think we can consider this "Accepted" and move on with the items discussed in the comments. Thanks again for putting this together!
|
|
||
| ### Negative Consequences <!-- optional --> | ||
|
|
||
| - Some devs may not know how to fix accessibility issues in Vue.js applications, and this may cause some friction in development or result in a number of rules being disabled. |
There was a problem hiding this comment.
That would be excellent – thank you!
TylerHendrickson
approved these changes
Nov 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note: If this is approved, I will create issues and update the disable comments w/ issue numbers before merging
Ticket #
Description
Part of a Design Record to add support for
eslint-plugin-vuejs-accessibility
Screenshots / Demo Video
Rules flagged
Testing
Automated and Unit Tests
Manual tests for Reviewer
Checklist