va-table: add scrollable prop #1428

janechodance · 2024-12-06T20:18:15Z

Chromatic

https://patch-bmt2-va-table-focusable-prop--65a6e2ed2314f7b8f98609d8.chromatic.com

Description

The Your VA Payments view has a table of payments that starts to extend outside the right edge of the viewport when I zoom my browser to 200-250%. This could prevent some users from seeing all of the data without zooming in to a higher resolution where the table adjusts to our responsive layout. Screenshot attached below.

Closes
department-of-veterans-affairs/va.gov-team#95134

QA Checklist

Component maintains 1:1 parity with design mocks
Text is consistent with what's been provided in the mocks
Component behaves as expected across breakpoints
Accessibility expert has signed off on code changes (if applicable. If not applicable provide reason why)
Designer has signed off on changes (if applicable. If not applicable provide reason why)
Tab order and focus state work as expected
Changes have been tested against screen readers (if applicable. If not applicable provide reason why)
New components are covered by e2e tests; updates to existing components are covered by existing test suite
Changes have been tested in vets-website using Verdaccio (if applicable. If not applicable provide reason why)

Screenshots

va.table.focusable.mov

va.table.focusable.mobile.mov

Acceptance criteria

QA checklist has been completed
Screenshots have been attached that cover desktop and mobile screens

Definition of done

Documentation has been updated, if applicable
A link has been provided to the originating GitHub issue (or connected to it via ZenHub)

jamigibbs

Hi @janechodance! 👋🏼 Do you mind adding a short description in the PR of the problem is and the solution being submitted here? That will help us understand a little faster instead of trying to work backwards through the linked ticket.

jamigibbs · 2024-12-09T14:43:18Z

packages/web-components/src/components/va-table/va-table-inner/va-table-inner.tsx

    const classes = classnames({
      'usa-table': true,
      'usa-table--stacked': stacked,
      'usa-table--borderless': tableType === 'borderless',
    });
    return (
-      <div>
+      <div tabIndex={isFocusable ? 0 : undefined}>


Is adding the tab-index=0 to this div essentially what you're wanting to use for keyboard tabbing into the table?

I'm going to tag in our a11y specialist Ryan on the PR because I sometimes get confused about when this is needed or not.

Hi! Yes. the goal is to add tabIndex=0 to this div so the keyboard using user can tab focus to the table and use arrow keys to horizontal scroll.

packages/web-components/src/components/va-table/va-table-inner/va-table-inner.tsx

janechodance · 2024-12-09T15:19:29Z

Hi @janechodance! 👋🏼 Do you mind adding a short description in the PR of the problem is and the solution being submitted here? That will help us understand a little faster instead of trying to work backwards through the linked ticket.

I added a description. Let me know if it makes sense. Thank you!

jamigibbs · 2024-12-09T16:43:36Z

Hi @janechodance! 👋🏼 Do you mind adding a short description in the PR of the problem is and the solution being submitted here? That will help us understand a little faster instead of trying to work backwards through the linked ticket.

I added a description. Let me know if it makes sense. Thank you!

Thanks @janechodance! I left a comment over on the ticket about trying to reproduce the original issue.

jamigibbs · 2024-12-10T17:17:01Z

Thanks again for your work here @janechodance!

We discussed the scrollable behavior internally and what we'd like to do is update the component so that the scrollable container and focus are combined into a single setting. This would make scrolling + focus opt-in for larger/dense tables and smaller tables that don't need scrolling or an extra focus tab could continue to opt-out. The default will continue to be stacked for smaller screens.

The implementation should be based on the USWDS guidelines for the scrollable table variation so we're hoping this PR could be updated with the following:

change the prop name from isFocusable to scrollable
update the prop description to say:
- When active, the table can be horizontally scrolled and is focusable.
default the prop to false
keep the tabindex=0 conditionally displaying on the container
also add the .usa-table-container--scrollable class to the same container when scrollable is true. It should already be available from the USWDS table module import.
remove the original overflow scrolling style added here. The .usa-table-container--scrollable styles should replace it.

Let us know if you have any concerns, thoughts, or issues with any of that though.

internal Slack discussion

janechodance · 2024-12-10T18:03:38Z

@jamigibbs Thank you! I will work on this and send an updated PR to you!

…lass

jamigibbs

I have just a few small things that I think would improve these updates but overall it looks great!

jamigibbs · 2024-12-11T17:14:02Z

packages/storybook/stories/va-table-uswds.stories.jsx

+  'rows': data,
+  'columns': defaultColumns,
+  'scrollable': true,


It looks like the WithPagination Storybook variation needs the scrollable prop too because it contains more content. I'm unable to scroll it now on small screens (before it stacks). Can you add that prop to that story too?

@janechodance I noticed one more thing from the USWDS example; the default behavior for scrollable tables is to also have stacked set to false.

Originally we were thinking that we'd let it stack on small screens but I'm going to tag @rsmithadhoc to get his thoughts about if we should add the stacked: false prop to our scrollable Storybook example so that it matches USWDS? 🤔 I think I'm fine either way but just looking for a sanity check.

@janechodance I noticed one more thing from the USWDS example; the default behavior for scrollable tables is to also have stacked set to false.

Originally we were thinking that we'd let it stack on small screens but I'm going to tag @rsmithadhoc to get his thoughts about if we should add the stacked: false prop to our scrollable Storybook example so that it matches USWDS? 🤔 I think I'm fine either way but just looking for a sanity check.

@jamigibbs I think I initially said make it stacked by default when we first did the table, just to eliminate any work from teams. However, I'm not too tied to that idea, so I could also go either way. I think the prop would give teams all the options at their disposal, so it might be the most flexible option.

@rsmithadhoc Thanks Ryan! Yes, I agree it would be good to see the flexibility with the options.

packages/web-components/src/components/va-table/va-table-inner/va-table-inner.tsx

janechodance added 2 commits December 6, 2024 14:54

add focusable attribute to va table

9da61cc

add tests

b546eef

janechodance added the patch Patch change in semantic versioning label Dec 6, 2024

janechodance marked this pull request as ready for review December 6, 2024 20:31

janechodance requested a review from a team as a code owner December 6, 2024 20:31

Merge branch 'main' into patch-bmt2-va-table-focusable-prop

7994d6a

jamigibbs reviewed Dec 9, 2024

View reviewed changes

jamigibbs changed the title ~~Patch bmt2 va table focusable prop~~ va-table: add focusable prop Dec 9, 2024

jamigibbs reviewed Dec 9, 2024

View reviewed changes

packages/web-components/src/components/va-table/va-table-inner/va-table-inner.tsx Outdated Show resolved Hide resolved

packages/web-components/src/components/va-table/va-table-inner/va-table-inner.tsx Outdated Show resolved Hide resolved

jamigibbs requested a review from rsmithadhoc December 9, 2024 14:50

jamigibbs mentioned this pull request Dec 9, 2024

[a11y-defect-3]: Payment History table extends past right edge of viewport at 200-250% browser zoom department-of-veterans-affairs/va.gov-team#95134

Closed

18 tasks

jamigibbs mentioned this pull request Dec 10, 2024

Striped and scrollable Table variations - Development department-of-veterans-affairs/vets-design-system-documentation#3448

Closed

14 tasks

janechodance added 5 commits December 10, 2024 16:29

update prop to scrollable and use 'usa-table-container--scrollable' c…

23e054a

…lass

update scrollable prop in storybook

9767769

update tests

b82fbfc

add scrollable to storybook

ae239e7

update va-table-inner.scss

de72c65

jamigibbs changed the title ~~va-table: add focusable prop~~ va-table: add scrollable prop Dec 11, 2024

jamigibbs reviewed Dec 11, 2024

View reviewed changes

jamigibbs added minor For a minor Semantic Version change and removed patch Patch change in semantic versioning labels Dec 11, 2024

janechodance added 3 commits December 11, 2024 12:34

update according to comments

9f828a1

Update va-table-uswds.stories.jsx

e4c4f57

Update va-table-uswds.stories.jsx

ff2ebf9

jamigibbs approved these changes Dec 12, 2024

View reviewed changes

Merge branch 'main' into patch-bmt2-va-table-focusable-prop

ef8bdc6

janechodance merged commit 5557a4f into main Dec 12, 2024
8 checks passed

janechodance deleted the patch-bmt2-va-table-focusable-prop branch December 12, 2024 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

va-table: add scrollable prop #1428

va-table: add scrollable prop #1428

janechodance commented Dec 6, 2024 •

edited

Loading

jamigibbs left a comment •

edited

Loading

jamigibbs Dec 9, 2024 •

edited

Loading

janechodance Dec 9, 2024

janechodance commented Dec 9, 2024

jamigibbs commented Dec 9, 2024

jamigibbs commented Dec 10, 2024 •

edited by janechodance

Loading

janechodance commented Dec 10, 2024

jamigibbs left a comment

jamigibbs Dec 11, 2024

jamigibbs Dec 11, 2024 •

edited

Loading

rsmithadhoc Dec 12, 2024

jamigibbs Dec 12, 2024

va-table: add scrollable prop #1428

va-table: add scrollable prop #1428

Conversation

janechodance commented Dec 6, 2024 • edited Loading

Chromatic

Description

QA Checklist

Screenshots

Acceptance criteria

Definition of done

jamigibbs left a comment • edited Loading

Choose a reason for hiding this comment

jamigibbs Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

janechodance Dec 9, 2024

Choose a reason for hiding this comment

janechodance commented Dec 9, 2024

jamigibbs commented Dec 9, 2024

jamigibbs commented Dec 10, 2024 • edited by janechodance Loading

janechodance commented Dec 10, 2024

jamigibbs left a comment

Choose a reason for hiding this comment

jamigibbs Dec 11, 2024

Choose a reason for hiding this comment

jamigibbs Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

rsmithadhoc Dec 12, 2024

Choose a reason for hiding this comment

jamigibbs Dec 12, 2024

Choose a reason for hiding this comment

janechodance commented Dec 6, 2024 •

edited

Loading

jamigibbs left a comment •

edited

Loading

jamigibbs Dec 9, 2024 •

edited

Loading

jamigibbs commented Dec 10, 2024 •

edited by janechodance

Loading

jamigibbs Dec 11, 2024 •

edited

Loading