-
Notifications
You must be signed in to change notification settings - Fork 56
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
Improve a11y in form example #1917
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for boosted ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
At least one thing to tackle before approval !
see with @MewenLeHo, ok for me ! |
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.
Haven't noticed before but seems that the Prefers not to say
is a bit too much for the width we provide. I don't know if it's mandatory to fix but might done later on.
General feeling: Seems maybe a bit too static for me but seems great as a first step! I'd like to see a real dynamic form later 😄
Still few changes to bring and we're good to go!
// Add the id of the corresponding invalid message to each invalid field | ||
invalidItems.forEach(element => { | ||
const linkedLabel = element.getAttribute('aria-labelledby') | ||
const closestInvalidFeedback = element.closest('.mb-3').querySelector('.invalid-feedback').getAttribute('id') |
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.
const closestInvalidFeedback = element.closest('.mb-3').querySelector('.invalid-feedback').getAttribute('id') | |
const closestInvalidFeedback = element.parentElement.querySelectorAll('.invalid-feedback') |
My point above but just recalling it here, We should have a way to display several feedbacks if needed.
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.
What happen when a field has multiple invalid feeddbacks will be discussed in the next specs meeting.
<div class="valid-feedback"> | ||
Looks good! | ||
</div> | ||
<div id="messageFeedback" class="invalid-feedback"> | ||
<div id="messageFeedback" class="invalid-feedback" role="alert"> |
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 should add the role="alert"
in the doc as well as in the migration guide.
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.
That's one of the point Vincent and I want to bring to the next specs meeting.
But it will probably be on Bootstrap's side first.
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.
Ah yes you're right, Bs first for documentation!
On hold until the incoming meeting about form validation in the next few days. |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
To do list:
|
Related issues
Closes #1599
Description
Improve accessibility by removing the
id
of the invalid message feedback in thearia-labelledby
attribute of eachinput
andselect
.This id is added on submit for invalid fields and remove when fields become valid.
Motivation & Context
Improve a11y for screen reader users.
Types of change
Live previews
Checklist
Contribution
Accessibility
Design
Development
Documentation
Checklist (for Core Team only)
After the merge