Skip to content
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

fix: [Table] Pagination input label's 'for' property needs to align with the input id #356

Merged
merged 6 commits into from
May 30, 2024

Conversation

meissadia
Copy link
Contributor

@meissadia meissadia commented May 24, 2024

Accessibility improvement

Changes

  • [Table] Propagate Table id to Pagination component
  • [Pagination] Utilize tableId to uniquely identify and label Pagination's input element

How to test this PR

  1. Load paginated table in an application (couldn't get ANDI to recognize the error in Storybook)
  2. Run ANDI accessibility tool
  3. Verify that it doesn't identify the following error
    • Element nested in <label> but label[for=m-pagination_current-page] does not match element [id=m-pagination_current-page-default

Screenshots

ANDI Before ANDI After
Screenshot 2024-05-24 at 3 32 30 PM Screenshot 2024-05-24 at 3 29 22 PM

Copy link

netlify bot commented May 24, 2024

Deploy Preview for cfpb-design-system-react ready!

Name Link
🔨 Latest commit 5aa9aae
🔍 Latest deploy log https://app.netlify.com/sites/cfpb-design-system-react/deploys/66562372c6da750008a7198f
😎 Deploy Preview https://deploy-preview-356--cfpb-design-system-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

- Uniquely identify pagination <input> elements
- Correctly associate pagination <input> labels with the input id
Copy link
Contributor

@shindigira shindigira left a comment

Choose a reason for hiding this comment

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

Have one suggestion to deal with this. Also, the id in the pagination story is showing as undefined.
Screen Shot 2024-05-25 at 10 30 40 AM

src/components/Pagination/Pagination.tsx Outdated Show resolved Hide resolved
@meissadia meissadia force-pushed the table__pagination-input__label-for branch from 5956806 to 281e57e Compare May 28, 2024 18:27
@meissadia meissadia requested a review from shindigira May 28, 2024 18:29
Copy link
Contributor

@shindigira shindigira left a comment

Choose a reason for hiding this comment

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

One more change and should be good to go.

src/components/Table/Table.tsx Show resolved Hide resolved
@meissadia meissadia requested a review from shindigira May 30, 2024 16:27
@meissadia meissadia dismissed shindigira’s stale review May 30, 2024 16:28

Provided context that I believe addresses the expressed concern.

Copy link
Contributor

@shindigira shindigira left a comment

Choose a reason for hiding this comment

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

LGTM!

src/components/Table/Table.tsx Show resolved Hide resolved
@meissadia meissadia merged commit 026da89 into main May 30, 2024
8 of 10 checks passed
@meissadia meissadia deleted the table__pagination-input__label-for branch May 30, 2024 17:52
meissadia added a commit to cfpb/sbl-frontend that referenced this pull request Jun 6, 2024
Close #568 

## Changes
- [Accessibility] Fix labelling issue in DSR Table/Pagination 
  - Dependent upon cfpb/design-system-react#356
- `Upload a new file` now a secondary button ([should be a link but I
couldn't get the styling overrides to play nice with one
another](#575))
- Update primary button label: `Download Report (CSV)`
- Remove green header border on Filing pages

## Screenshots
|Change|Screenshot|
|---|---|
|Button label (Warnings)|<img width="739" alt="Screenshot 2024-05-31 at
4 04 11 PM"
src="https://github.com/cfpb/sbl-frontend/assets/2592907/be87effb-1174-446a-89e8-44433b0a1c71">|
|Button label (Errors)|<img width="767" alt="Screenshot 2024-05-31 at 4
09 02 PM"
src="https://github.com/cfpb/sbl-frontend/assets/2592907/a5b4b6d1-f4c7-4be5-8ada-5f9d596026ea">|
|Remove green header border on filing|<img width="1230" alt="Screenshot
2024-06-03 at 9 54 54 AM"
src="https://github.com/cfpb/sbl-frontend/assets/2592907/e404f67b-3ff6-46a6-825f-fab60e574260">|
|Accessibility for pagination: input's `for` label aligns with table
id|<img width="974" alt="Screenshot 2024-06-03 at 9 53 03 AM"
src="https://github.com/cfpb/sbl-frontend/assets/2592907/35732b11-95ac-4627-a12e-00331896848b">|

---------

Co-authored-by: Bill Himmelsbach <whimmels@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants