accessibility updates #2618
accessibility updates #2618
Conversation
thetif
requested review from
radavis,
beparticular,
CSwartzHMA,
jeromeleecms and
tbolt
and removed request for
CSwartzHMA
|
This deploy was cleaned up. |
There was a problem hiding this comment.
- "Agree and Continue Screen" The skip link popup appears, but it doesn't take me anywhere. Shouldn't this take me to the "Agree and Continue Button?"
- "Login Form" The skip link popup appears, but it takes me to a nebulous area where tabbing / back tabbing takes me back to browser address window. Where should I expect for it to take me? Screen reader just reads the entire form.
- "Dashboard" The skip link popup appears and takes me to the main header page (screen reader starts as intended)
- "APD Builder Page" The skip link pop appears, and it takes me to the top of the builder.
Will put a hold on the rest of this review per Dev team.
|
tried to resolve these issues, can you try it again? |
There was a problem hiding this comment.
These updates look good. The skip nav behavior is baffling as it is inconsistent across browsers / operating systems.
For the Key Personnel, I believe the "Edit / Remove" needs to be associated with the person added. I can understand if that needs to be part of a larger design update though
| ); | ||
|
|
||
| const previousComponent = !previousLink ? null : ( | ||
| <Link | ||
| aria-labelledby={previousLabelId} | ||
| aria-label={`Previous to ${previousText}`} |
There was a problem hiding this comment.
I think this should be OK but im curious if @beparticular has any thoughts on this language.
There was a problem hiding this comment.
@beparticular suggested relabeling the button to Back instead of Previous
web/src/containers/Login.js
Outdated
| <UpgradeBrowser /> | ||
| <CardForm | ||
| id="start-main-content" |
There was a problem hiding this comment.
Another option is to have the <main> wrap the <CardForm>. I can understand why we wouldn't want to consider the upgrade browser warning as part of the main content though.
There was a problem hiding this comment.
yeah that was my thinking, do you think it's better to wrap the whole page?
There was a problem hiding this comment.
From https://developer.mozilla.org/en-US/docs/Web/HTML/Element/main
The HTML
<main>element represents the dominant content of the of a document. The main content area consists of content that is directly related to or expands upon the central topic of a document, or the central functionality of an application.
The content of a
<main>element should be unique to the document. Content that is repeated across a set of documents or document sections such as sidebars, navigation links, copyright information, site logos, and search forms shouldn't be included unless the search form is the main function of the page.
I think wrapping the whole page (except the navigation in this case) makes sense.
web/src/styles/scss/layout.scss
Outdated
| height: 3rem; | ||
| line-height: 3rem; | ||
| padding: 0 0.5rem; | ||
| padding: 1rem; | ||
| width: fit-content; |
There was a problem hiding this comment.
I'm not sure if this would have an impact but IE doesn't support fit-content
There was a problem hiding this comment.
it doesn't really work anyway
… personnel name to the aria-label on edit and delete buttons in key personnel
thetif
requested review from
beparticular
and removed request for
beparticular
Fixes the first 3 issues in #2607
This pull request changes...
This pull request is ready to merge when...
- [ ] The change has been documented- [ ] Associated OpenAPI documentation has been updatedThis feature is done when...