Skip to content

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 2 times, most recently 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.

👍

@Ovgodd
Copy link
Collaborator Author

Ovgodd commented Sep 5, 2025

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

yes this is a good idea ! and create a specific modal folder too.

@Ovgodd Ovgodd force-pushed the fix/nvda-close-button branch from ba5ff2d to f3c690d Compare September 5, 2025 07:04
Copy link

github-actions bot commented Sep 5, 2025

Size Change: -834 B (-0.02%)

Total Size: 3.65 MB

Filename Size Change
apps/impress/out/_next/static/chunks/pages/_app.js 412 kB -1.05 kB (-0.25%)
apps/impress/out/_next/static/e0e58088/_buildManifest.js 0 B -867 B (removed) 🏆
apps/impress/out/_next/static/efaefbde/_buildManifest.js 869 B +869 B (new file) 🆕

compressed-size-action

@Ovgodd Ovgodd requested a review from AntoLC September 5, 2025 07:53
@Ovgodd Ovgodd force-pushed the fix/nvda-close-button branch 3 times, most recently from abf75ae to c6f5889 Compare September 5, 2025 15:00
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 c6f5889 to 084d0c1 Compare September 5, 2025 15:49
@Ovgodd Ovgodd merged commit 084d0c1 into main Sep 5, 2025
21 of 22 checks passed
@Ovgodd Ovgodd deleted the fix/nvda-close-button branch September 5, 2025 17:04
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.

2 participants