UI: Avoid large animation for reduced motion users#33530
Conversation
|
View your CI Pipeline Execution ↗ for commit f2a087e
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughAdds detection of the user's Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes ✨ Finishing touches
Comment |
ghengeveld
left a comment
There was a problem hiding this comment.
I noticed this in review and did not make a remark about it because you're only animating the border color, which isn't jarring / nausea inducing since there is no motion involved, so I don't think it's necessary to disable it for reduced motion. If you feel strongly about it, I'm okay with disabling it, but I'd rather not for simplicity's sake.
Yeah I'm not entirely confident it's a problem to have it (other than from a strict compliance standpoint). I'll ask around for third-party opinions. |
I've heard from someone who has done research on SC 2.3.3 and they believe it is better to turn the animation off when prefers-reduced-motion is on. The user impact if we didn't would be minor in practice, but it would likely be flagged as a violation and they don't think the feature is important enough to be preserved. |
At this point it's a trade-off between strict compliance regardless of practical impact, and the maintainability of it on our end. In my opinion, because we are running an open source product with all the complexities that entails, it is not worth the trade-off. If we disable this particular color-fade animation, we are going to have to disable it in all other instances as well. We could do a blanket CSS rule to simply disable all animations/transitions when prefers-reduced-motion is set, but there's a good chance that will lead to unforeseen consequences, which go undetected because we hardly ever QA with prefers-reduced-motion. Nevertheless, if we do want to be strict about it, a blanket rule is what I would prefer over individual checks. |
A blanket rule could have unintended consequences indeed. And needless to say we don't have a good way to handle that right now with our styling stack; though we could have CSS+JS variables drive animation durations in the future, which would actually allow us to systematise animation disabling. I also wouldn't want to do that if we don't have an automated (labour-free) QA workflow to protect us from errors. Ultimately we can just close this PR. It would leave a small WCAG violation behind but I've been reassured that the practical consequences for users would be negligible (which we didn't know prior to starting that conversation!). |
8c3567e to
f2a087e
Compare
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 192 | 192 | 0 |
| Self size | 75 KB | 75 KB | 🎉 -6 B 🎉 |
| Dependency size | 32.24 MB | 32.25 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 49 | 49 | 0 |
| Self size | 20.30 MB | 20.32 MB | 🚨 +21 KB 🚨 |
| Dependency size | 16.52 MB | 16.52 MB | 🎉 -4 B 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/ember
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 196 | 196 | 0 |
| Self size | 15 KB | 15 KB | 🚨 +12 B 🚨 |
| Dependency size | 28.96 MB | 28.97 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 538 | 538 | 0 |
| Self size | 646 KB | 646 KB | 🎉 -10 B 🎉 |
| Dependency size | 59.22 MB | 59.73 MB | 🚨 +505 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 127 | 127 | 0 |
| Self size | 1.12 MB | 1.12 MB | 🚨 +14 B 🚨 |
| Dependency size | 21.82 MB | 22.32 MB | 🚨 +496 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 159 | 159 | 0 |
| Self size | 30 KB | 30 KB | 🎉 -10 B 🎉 |
| Dependency size | 23.00 MB | 23.61 MB | 🚨 +610 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 117 | 117 | 0 |
| Self size | 35 KB | 35 KB | 🎉 -26 B 🎉 |
| Dependency size | 19.62 MB | 20.11 MB | 🚨 +496 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 278 | 278 | 0 |
| Self size | 24 KB | 24 KB | 🎉 -10 B 🎉 |
| Dependency size | 44.13 MB | 44.64 MB | 🚨 +505 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/server-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 204 | 204 | 0 |
| Self size | 16 KB | 16 KB | 🎉 -10 B 🎉 |
| Dependency size | 33.49 MB | 33.50 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 183 | 183 | 0 |
| Self size | 775 KB | 775 KB | 🚨 +219 B 🚨 |
| Dependency size | 67.38 MB | 67.47 MB | 🚨 +92 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 176 | 176 | 0 |
| Self size | 30 KB | 30 KB | 🎉 -40 B 🎉 |
| Dependency size | 65.95 MB | 66.05 MB | 🚨 +92 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 1000 KB | 1000 KB | 🎉 -8 B 🎉 |
| Dependency size | 36.82 MB | 36.84 MB | 🚨 +21 KB 🚨 |
| Bundle Size Analyzer | node | node |
@storybook/preset-react-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 170 | 170 | 0 |
| Self size | 18 KB | 18 KB | 🚨 +18 B 🚨 |
| Dependency size | 31.26 MB | 31.28 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 57 | 57 | 0 |
| Self size | 732 KB | 1.23 MB | 🚨 +494 KB 🚨 |
| Dependency size | 12.94 MB | 12.94 MB | 🚨 +2 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
What I did
I forgot to guard the landmark region animation for prefers-reduced-motion users.
Checklist for Contributors
Testing
ø
Manual testing
Enable reduced motion emulation

Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
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 publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.