fix: accessibility in etiquette#876
Conversation
There was a problem hiding this comment.
Pull request overview
This PR targets accessibility improvements for the “etiquette” (Shopify external module), and tightens CI accessibility expectations.
Changes:
- Increased the clickable size (padding/height) of the Progress and Disclaimer icon buttons.
- Conditionally removed the Disclaimer UI when the
randomizebutton is present in the inline etiquette. - Raised the Lighthouse accessibility score threshold in the E2E workflow.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/components/externalModules/shopify/Progress.module.css | Enlarges the Progress control button sizing to improve tap/target size. |
| src/components/externalModules/shopify/Equivalent.tsx | Hides Disclaimer when randomize is provided (inline etiquette path). |
| src/components/externalModules/shopify/Disclaimer.module.css | Enlarges the Disclaimer control button sizing to improve tap/target size. |
| .github/workflows/e2e.yml | Raises Lighthouse minAccessibilityScore threshold to 100. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| // On n'affiche le dislaimer que si il n'y a pas le bouton randomize, pour des soucis d'accessibilité | ||
| // Aujourd'hui c'est juste sur le site donc pas de soucis. A voir comment on fera plus tard | ||
| !randomize && ( | ||
| <Disclaimer | ||
| language={language} | ||
| comparisons={comparisons} | ||
| baseValue={preciseValue} | ||
| id={animated ? 'animated' : 'static'} | ||
| /> | ||
| ) | ||
| } |
There was a problem hiding this comment.
Conditionally removing the when randomize is provided changes the feature set of the inline “etiquette”: users lose access to the content/info panel in the randomized variant, while the column variant (ColumnEquivalent) still shows the disclaimer even when randomize exists. If the goal is to resolve an accessibility issue (e.g. overlapping controls or an ARIA relationship problem), it’d be better to fix that underlying issue while keeping the disclaimer available, and keep behavior consistent between inline/column renderings.
| { | |
| // On n'affiche le dislaimer que si il n'y a pas le bouton randomize, pour des soucis d'accessibilité | |
| // Aujourd'hui c'est juste sur le site donc pas de soucis. A voir comment on fera plus tard | |
| !randomize && ( | |
| <Disclaimer | |
| language={language} | |
| comparisons={comparisons} | |
| baseValue={preciseValue} | |
| id={animated ? 'animated' : 'static'} | |
| /> | |
| ) | |
| } | |
| <Disclaimer | |
| language={language} | |
| comparisons={comparisons} | |
| baseValue={preciseValue} | |
| id={animated ? 'animated' : 'static'} | |
| /> |
| // On n'affiche le dislaimer que si il n'y a pas le bouton randomize, pour des soucis d'accessibilité | ||
| // Aujourd'hui c'est juste sur le site donc pas de soucis. A voir comment on fera plus tard |
There was a problem hiding this comment.
The new inline comment has spelling/grammar issues (“dislaimer”, “si il”, etc.) and reads like a temporary rationale (“pas de soucis… plus tard”) rather than actionable documentation. Please correct the wording (or replace with a proper TODO linked to an issue) so future maintainers understand the concrete accessibility constraint being addressed.
| // On n'affiche le dislaimer que si il n'y a pas le bouton randomize, pour des soucis d'accessibilité | |
| // Aujourd'hui c'est juste sur le site donc pas de soucis. A voir comment on fera plus tard | |
| // N'afficher le disclaimer que lorsqu'il n'y a pas de bouton de randomisation, | |
| // afin d'éviter un problème d'accessibilité lorsque les deux éléments sont présents. |
b7aa64d to
b6eda94
Compare
b6eda94 to
355b0eb
Compare
No description provided.