-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
App banners: Don't play animation if reduced motion's enabled #67826
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Async-loaded Components (~68 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than my comment, I don't have any concerns with this approach 👍
client/blocks/app-banner/index.jsx
Outdated
const reducedMotion = window.matchMedia( '(prefers-reduced-motion: reduce)' ).matches; | ||
|
||
if ( reducedMotion ) { | ||
animation.goToAndStop( 145, true ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only concern is whether the animations would ever get longer than this 😁
Maybe there's a way to do it dynamically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we may be able to reference animation.totalFrames
to avoid the 145
reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks both, I did experiment with animation.totalFrames
but it only ever returned a value of 0
in my testing. 🤔 I'll have a further dive into this as I'm sure there must be some approach for getting the last frame that I'm missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. It looks like we may need to await configuration ready. Also, I wonder if it makes sense to avoid autoplaying when the user prefers reduced motion.
diff --git a/client/blocks/app-banner/index.jsx b/client/blocks/app-banner/index.jsx
index 52b4c522b4..3e165828da 100644
--- a/client/blocks/app-banner/index.jsx
+++ b/client/blocks/app-banner/index.jsx
@@ -255,18 +255,20 @@ export class AppBanner extends Component {
function BannerIcon( { icon } ) {
useEffect( () => {
+ const reducedMotion = window.matchMedia( '(prefers-reduced-motion: reduce)' ).matches;
+
const animation = lottie.loadAnimation( {
container: document.querySelector( '.app-banner__icon' ),
renderer: 'svg',
loop: false,
- autoplay: true,
+ autoplay: ! reducedMotion,
path: icon,
} );
- const reducedMotion = window.matchMedia( '(prefers-reduced-motion: reduce)' ).matches;
-
if ( reducedMotion ) {
- animation.goToAndStop( 145, true );
+ animation.addEventListener( 'config_ready', () => {
+ animation.goToAndPlay( animation.totalFrames, true );
+ } );
}
return () => animation.destroy();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks David! I've gone ahead to implement your suggestion in f61ab8a.
Also, I wonder if it makes sense to avoid autoplaying when the user prefers reduced motion.
I think so, it was necessary to have this enabled when using goToAndStop
, but not necessary with goToAndPlay
, so I've followed your suggestion. 🙇♀️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little comment about the BannerIcon
component: instead of globally querying an element with document.querySelector( '.app-banner__icon' )
, it should get the element locally, with a ref.
const iconEl = useRef();
useEffect( () => {
loadAnimation( { container: iconEl.current } );
}, [] );
return <div ref={ iconEl } className="app-banner__icon" />;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 LGTM. Tested the latest revision in Chrome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This change takes a user's "reduced motion" preference into account before displaying an animation in the app banners. The animation will not play if the "reduced motion" is enabled.
Proposed Changes
Following the feedback in #67013 (comment), this PR takes a user's "reduced motion" preference into account before displaying an animation in the app banners. The animation will not play if the "reduced motion" is enabled.
window.matchMedia
is used to detect whenprefers-reduced-motion: reduce
is set.goToAndPlay
function is then used to stop the animation on the last frame. Note, this is necessary as Lottie does not take motion preferences into account by default.ltr.mov
reduced-motion.mov
Testing Instructions
Prerequisites:
Testing steps:
Pre-merge Checklist