UI: Replace IconButton with Button, ToggleButton & Select, add ARIA labels#31702
UI: Replace IconButton with Button, ToggleButton & Select, add ARIA labels#31702Sidnioulz wants to merge 53 commits into
Conversation
There was a problem hiding this comment.
48 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile
|
View your CI Pipeline Execution ↗ for commit 763dfd8
☁️ Nx Cloud last updated this comment at |
22cc7dc to
104d77f
Compare
4b137a4 to
f1b8117
Compare
|
To do for me:
|
fc44f9f to
2e6537d
Compare
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 48 | 48 | 0 |
| Self size | 30.74 MB | 30.78 MB | 🚨 +39 KB 🚨 |
| Dependency size | 17.61 MB | 17.61 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 204 | 204 | 0 |
| Self size | 879 KB | 879 KB | 0 B |
| Dependency size | 81.72 MB | 81.76 MB | 🚨 +39 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 173 | 173 | 0 |
| Self size | 35 KB | 35 KB | 0 B |
| Dependency size | 76.79 MB | 76.83 MB | 🚨 +39 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 49 | 49 | 0 |
| Self size | 1.52 MB | 1.52 MB | 🎉 -30 B 🎉 |
| Dependency size | 48.35 MB | 48.39 MB | 🚨 +39 KB 🚨 |
| Bundle Size Analyzer | node | node |
We've since had several work sessions and cleaned up an enormous amount of issues :)
da4db5f to
268805e
Compare
268805e to
3a794d4
Compare
|
I added |
Sure thing! I wanted to squash this PR anyway due to the high amount of low-quality commit messages. Just noting it here to have a trace when the time comes. @ghengeveld could we consider merging to an |
|
@Sidnioulz Yes, sounds like a good idea to merge your a11y work into a single branch so we can merge it all in one go. |
|
Closing the PR as code was manually applied to https://github.com/storybookjs/storybook/tree/a11y-consolidation. |
Closes #21578
Closes #24073
Closes #31261
Changes to UI
addon-a11y
addon-backgrounds
addon-docs
addon-pseudostates
addon-themes
addon-vitest
addon-interactions
statusrole to the status button for rudimentary live reporting (will need review after we have better aria live utils)Mobile layout
toolbarroleNotifications
Addon panel
Toolbar
Sidebar
Onboarding
Settings
Changes to docs
Changes to existing components
Button
ariaLabelpropfalseto convey that the button does not need labellingdescriptionprop (name TBC) that creates a hidden region for an aria-describedby attr when we want to tell screenreader users how a feature works in more detailtooltipprop, set by default toariaLabelifariaLabelis notfalseshortcutprop that feeds intoaria-keyshortcutsand gets appended to the tooltip for the buttonactiveprop as buttons with an active state must convey a specific semanticIconButton
Modal
New components
InteractiveTooltipWrapper
ToggleButton
activeprop that toggle a feature on or offswitchrole to convey that it is pressed, and requires apressedboolean prop to bet set.Select
listboxrole with options to be selectedTO DO
Decide on whether to improve the vocabulary for addon tool declarative selects (missing aria label)(now feeling redundant with title, let's sit on this one a bit and see if anyone actually requests a change)Known issues
Click behaviour on tooltips
This PR introduces a regression with tooltips. They can no longer be closed by clicking outside. This is due to a Popper limitation to be adressed in #32249
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Documentation
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>Greptile Summary
This PR represents a comprehensive accessibility overhaul of Storybook's UI components, systematically replacing IconButton components with a new unified Button API that enforces proper accessibility through mandatory aria-label props, integrated tooltip functionality, and keyboard shortcut support.
Key Changes
Component Architecture Redesign:
ariaLabelprop (string orfalse), newshortcutprop for keyboard shortcuts,tooltipprop, anddescriptionprop for detailed accessibility informationAccessibility Improvements:
ariaLabelpropsrole="switch"witharia-checked, Select usesrole="listbox"with proper option semanticsMigration Impact:
activeprop from Button in favor of semantic alternatives, deprecated IconButtonIntegration with Existing Codebase
The changes integrate seamlessly with Storybook's component ecosystem by:
variantandpaddingprops that replicate previous IconButton stylingConfidence score: 4/5