Skip to content

✨(frontend) make delete buttons nvda-accessible; add close to title a… #1281

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Ovgodd
Copy link
Collaborator

@Ovgodd Ovgodd commented Aug 11, 2025

Purpose

Improve NVDA accessibility by replacing non-labeled modal close buttons from cunningham-react with custom buttons in the title prop and explicit aria-labels

Proposal

  • Hide default modal close buttons
  • Add custom close buttons with context-specific aria-labels in title prop
  • Ensure consistent patterns across all modal components

@Ovgodd Ovgodd force-pushed the fix/nvda-close-button branch from 9a13513 to 8edc540 Compare August 11, 2025 09:37
@Ovgodd Ovgodd marked this pull request as ready for review August 11, 2025 09:37
@Ovgodd Ovgodd requested a review from AntoLC August 11, 2025 09:37
@Ovgodd Ovgodd self-assigned this Aug 11, 2025
@Ovgodd Ovgodd force-pushed the fix/nvda-close-button branch from 8edc540 to cacc2df Compare August 11, 2025 09:42
add aria-labels and include close button in title prop so NVDA announces actions

Signed-off-by: Cyril <c.gromoff@gmail.com>
@Ovgodd Ovgodd force-pushed the fix/nvda-close-button branch from cacc2df to 076e76f Compare August 11, 2025 12:37
Copy link
Collaborator

@AntoLC AntoLC left a comment

Choose a reason for hiding this comment

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

Can you create a component ButtonCloseModal in src/components to have it commun to all the modals ?

Maybe add a modals folder with everything related to modal in it, wdyt ?
image

@@ -72,6 +72,7 @@ test.describe('Home page', () => {
await page.waitForLoadState('domcontentloaded');

// Wait a bit more for the responsive store to be initialized
// eslint-disable-next-line playwright/no-wait-for-timeout
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

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

Successfully merging this pull request may close these issues.

2 participants