fix(filter-chip): replace aria-pressed with CSS class for menu button selected state#580
Conversation
🦋 Changeset detectedLatest commit: 4660537 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
We probably need updates on the React and Marko sides as well, right @HenriqueLimas and @LuLaValva ? |
There was a problem hiding this comment.
Pull request overview
This PR updates the Skin filter-chip “menu button” pattern to avoid misusing aria-pressed (which implies a self-toggling button) by switching the visual selected state to a CSS modifier class and updating Storybook examples accordingly.
Changes:
- Adds
.filter-chip--selectedsupport infilter-chip.scssfor selected styling/animation alongside the existing[aria-pressed="true"]selectors. - Updates the menu-button filter-chip Storybook stories to remove
aria-pressed, apply.filter-chip--selectedfor selected examples, and add clipped “filter applied” text. - Adds a changeset for
@ebay/skin(patch).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/skin/src/sass/filter-chip/stories/menu-button-chip.stories.js | Updates menu filter-chip Storybook markup to stop using aria-pressed and provide SR-friendly selected-state text. |
| packages/skin/src/sass/filter-chip/filter-chip.scss | Adds .filter-chip--selected as an alternative selected-state selector for buttons (and animated variant). |
| .changeset/loose-lizards-drop.md | Declares a patch release note for the Skin package. |
| export default { title: "Skin/Filter Chip/Menu" }; | ||
|
|
||
| export const collapsed = () => ` | ||
| <button class="filter-chip" aria-expanded="false" aria-pressed="false"> | ||
| <button class="filter-chip" aria-expanded="false"> | ||
| <span class="filter-chip__text">Football</span> |
There was a problem hiding this comment.
The PR description/issue notes indicate menu filter-chip examples should drop aria-pressed and rely on .filter-chip--selected + clipped text. However, the website docs example at src/routes/_index/components/filter-chip/css+page.marko (Menu Button Chip section) still uses aria-pressed on the menu button markup, which leaves the documented pattern inconsistent with the updated Skin stories/CSS. Update the docs example to match the new guidance.
|
|
||
| export const collapsed = () => ` | ||
| <button class="filter-chip" aria-expanded="false" aria-pressed="false"> | ||
| <button class="filter-chip" aria-expanded="false"> |
There was a problem hiding this comment.
Consider adding type="button" to these <button> examples. Without an explicit type, buttons default to submit when placed inside a form, which can cause unexpected behavior for consumers copying the markup from Storybook.
saiponnada
left a comment
saiponnada
left a comment
There was a problem hiding this comment.
Thanks @dididy for taking this up. Few things pending,
- We need examples in skin stories for expressive variant(with
aria-pressedbut noaria-expanded) - Also update the docs under
filter-chip/css+page.marko - Implementations on both Marko and React and add corresponding tests.
If you want to close this out, we’re happy to take it on—especially if working across the frameworks isn’t convenient on your side.
|
Addressed all the review feedback!
The build has some |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #580 +/- ##
=======================================
Coverage ? 41.05%
=======================================
Files ? 2114
Lines ? 11350
Branches ? 1640
=======================================
Hits ? 4660
Misses ? 6320
Partials ? 370 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
saiponnada
left a comment
saiponnada
left a comment
There was a problem hiding this comment.
Overall LGTM! Thanks for making these changes. We have minor knits to fix and good to merge IMO.
|
All addressed. thanks for the review! 🙏 |
Description
Notes
Menu filter-chips used
aria-pressedto show selected state. That's the wrong attribute here -aria-pressedis for buttons that toggle their own state on/off, but a menu chip reflects a selection made inside another UI element. Screen readers were announcing a pressed state that had nothing to do with the button itself.Two changes:
filter-chip--selectedclass as a selector for selected state and animation, alongside the existingaria-pressed="true". Menu chips can now droparia-pressedwithout losing visual feedback.aria-pressedfrom all menu button chip examples, addedfilter-chip--selectedclass and<span class="clipped">filter applied</span>so screen readers announce filter state correctly.Non-menu chips (no
aria-expanded) still usearia-pressedas before.As-Is
<button class="filter-chip" aria-pressed="true">To-Be
<button class="filter-chip filter-chip--selected">+<span class="clipped">filter applied</span>Screenshots
Checklist
Checklist