Skip to content

Conversation

mejiaj
Copy link
Contributor

@mejiaj mejiaj commented Feb 21, 2024

Summary

Address a11y issue in table demos on website. For now, a simple table will be used and an example of a complex table will come in the future.

Breaking change

This is not a breaking change.

Related issue

Closes uswds/uswds-site#2261.

Related pull requests

Changelog entry: uswds/uswds-site#2501.

Preview link

Preview link:

Problem statement

Currently, there's an A11Y issue with table example in scrollable variant on the website. The year header for 2019 doesn't have associated data cells underneath.

Solution

Replaced complex table with simpler table used in default variant.

Major changes

Extended default usa-table.twig template in favor of separate hardcoded usa-table--scrollable.twig template [84256e3].

Scrollable template changes

<!-- usa-table--scrollable.twig -->
+ {% extends "../usa-table.twig" %}
- <div class="usa-table-container--scrollable" tabindex="0">
-  <table class="usa-table {{ modifier }}">
-    {% if caption %}
-    <caption>{{ caption }}</caption> 
…etc

Testing and review

  1. All tables in scrollable table preview links have usa-table-container--scrollable wrapper.
  2. Confirm there are no regressions in other table variants (ex: wrong markup, classes, or visual regressions).
  3. Confirm npm run build:html compiles correctly for scrollable and all variants.

Checklist

  • Scrollable tables have correct wrapper usa-table-container--scrollable.
  • Zero regressions in other table variants.
  • Compiled table templates in html-templates/ compile correct markup.

Copy link
Contributor Author

@mejiaj mejiaj Feb 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same table data from usa-table.json.

This allows us to:

  1. Reliably compile HTML templates for website.
  2. Dynamically create table using default usa-table.twig.
  3. Get rid of the static usa-table--scrollable.twig template.

Note

You can re-use usa-table.json in StorybookJS, but causes regressions on HTML templates. On compile (npm run build:html) it will result in empty scrollable table variants.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for that note! It is helpful to know why it was set up this way.

I would be interested to investigate why and see if there is a way in the future to re-use some of this table JSON since we repeat so much, but that is out of scope for this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed anymore since we're taking advantage of the dynamically generated table in usa-table.twig.

Copy link
Contributor Author

@mejiaj mejiaj Feb 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scrollable markup was hardcoded to mimic previous template when the StorybookJS control was toggled. Not a recommended pattern moving forward.

Scrollable toggle
image

Copy link
Contributor Author

@mejiaj mejiaj Feb 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Inspect all compiled HTML templates for table to ensure there aren't regressions on site.

Should generate markup in html-templates directory for website.

Simple scrollable table
Notice, multi-column headers are gone.


Compiled HTML - usa-table--scrollable.html

<!-- html-templates/usa-table--scrollable.html -->
<div class="usa-table-container--scrollable" tabindex="0">
  <table class="usa-table">
    <caption>
      Scrollable table
    </caption>
    <thead>
      <tr>
        <th scope="col">Document title</th>
        <th scope="col">Description</th>
        <th scope="col">Year</th>
      </tr>
    </thead>
    <tbody>
      <tr>
        <th scope="row">Declaration of Independence</th>
        <td>
          Statement adopted by the Continental Congress declaring independence
          from the British Empire.
        </td>
        <td>1776</td>
      </tr>
      <tr>
        <th scope="row">Bill of Rights</th>
        <td>
          The first ten amendments of the U.S. Constitution guaranteeing rights
          and freedoms.
        </td>
        <td>1791</td>
      </tr>
      <tr>
        <th scope="row">Declaration of Sentiments</th>
        <td>
          A document written during the Seneca Falls Convention outlining the
          rights that American women should be entitled to as citizens.
        </td>
        <td>1848</td>
      </tr>
      <tr>
        <th scope="row">Emancipation Proclamation</th>
        <td>
          An executive order granting freedom to slaves in designated southern
          states.
        </td>
        <td>1863</td>
      </tr>
    </tbody>
  </table>
</div>

@mejiaj mejiaj added this to the uswds 3.8.0 milestone Feb 22, 2024
@mejiaj mejiaj assigned mejiaj and unassigned mejiaj Feb 22, 2024
mejiaj pushed a commit to uswds/uswds-site that referenced this pull request Feb 23, 2024
mejiaj pushed a commit to uswds/uswds-site that referenced this pull request Feb 23, 2024
@mejiaj mejiaj marked this pull request as ready for review February 23, 2024 14:50
Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I've performed the following tests:

  • Check that the scrollable table markup has usa-table-container--scrollable wrapper (Checked in both storybook and html-templates)
  • Confirm no visual regressions in any table variant
  • Perform a code diff on the html-template markup for all table variants to confirm no changes to the markup
  • Confirm table styles look good when this branch is installed on uswds-site
  • Confirm there are no conflicts with sticky table header PR (#5420)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for that note! It is helpful to know why it was set up this way.

I would be interested to investigate why and see if there is a way in the future to re-use some of this table JSON since we repeat so much, but that is out of scope for this issue.

Copy link
Contributor

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for flagging the html build error when you include usa-table.json. I was hoping to be able to do the same!

Testing checklist

  • Scrollable tables have the correct wrappers
  • Zero regressions in other table markup
  • html-templates have zero regressions
  • Installing on site properly updates component previews and component code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

USWDS-Site - Bug: Revise Scrollable Table Example
4 participants