-
Notifications
You must be signed in to change notification settings - Fork 20
va-table: add caption summary and focus props #1907
base: main
Are you sure you want to change the base?
Changes from all commits
a310f67
59df18e
3571b77
77547e2
c761906
6225984
c56bcfc
8dc40f6
81a8664
3ef80a8
286d923
ed8257c
8654215
0045eec
66f7bbe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,9 @@ | |
| @use 'usa-table/src/styles/usa-table'; | ||
| @use 'uswds-helpers/src/styles/usa-sr-only'; | ||
| @use "~@department-of-veterans-affairs/css-library/dist/tokens/scss/variables" as *; | ||
|
|
||
| @import '~@department-of-veterans-affairs/css-library/dist/stylesheets/utilities.css'; | ||
| @import '../../../mixins/focus'; | ||
|
|
||
| :host { | ||
| td slot::slotted(span:empty)::before, | ||
|
|
@@ -53,10 +55,6 @@ | |
| top: 50%; | ||
| transform: translate(0, -50%); | ||
| text-align: center; | ||
|
|
||
| &:focus { | ||
| outline: 2px solid var(--vads-color-action-focus-on-light); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -70,12 +68,14 @@ | |
| } | ||
|
|
||
| caption { | ||
| text-align: left; | ||
| padding: 0 0 0.313rem; | ||
| font-weight: 700; | ||
| font-size: 1.25rem; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| font-family: var(--font-serif); | ||
| margin-bottom: 0.75rem; | ||
| margin: 0.25rem; | ||
| #summary { | ||
| display: block; | ||
| font-weight: normal; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jamigibbs I think the styling is good. I'm not sure if that's the best placement for the results text though? Should it be closer to the pagination controls? 🤔 That might complicat focus order though. This is just a hunch, but I can do some research if necessary. Before doing that, I wonder if this be more of an accessibility question though. @amyleadem @jeana-adhoc Your thoughts? Here's a quick mock up moving the results total to be near the pagination controls:
(It will need a little more space if we go this direction.)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think @jamigibbs initial placement makes sense. It's similar to how we manage search results.
After triggering a pagination change, we need to announce that change and send focus somewhere, and we wouldn't want to focus directly above the pagination. @amyleadem Would be curious if you have other thoughts about this. |
||
| } | ||
| &:focus { | ||
| @include focus-style; | ||
| } | ||
| } | ||
|
|
||
| @media screen and (max-width: $medium-screen) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| @mixin focus-style { | ||
| outline: 2px solid var(--vads-color-action-focus-on-light); | ||
| outline-offset: 2px; | ||
| z-index: 2; | ||
| } | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: I created this Sass mixin because the single stylesheet approach for focus style has become unwieldy. I am going to create a follow-up ticket for us to look at switching to using a direct mixin in the other places the original stylesheet is referencing. |
||




Uh oh!
There was an error while loading. Please reload this page.
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.
Howdy @department-of-veterans-affairs/platform-design-system-a11y reviewer! 👋🏼 I added a prop for focusing the
captionelement as-needed and it appears to be functioning as intended but my VO testing resulted in some duplicate readouts when I integrated it with this pagination example in Storybook.Is it possible that this isn't the correct a11y approach for using pagination with va-table?
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.
I'll take a look - I heard it duplicated in VO too (but sometimes that just.. happens). I'm going to listen in NVDA & JAWS too, then come back and see if this might just be a VO quirk.
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.
Thanks @jeana-adhoc! Based on our convo, I've updated the description of the PR with a11y notes to explain our approach. Feel free to edit or add if you'd like!