-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
reduceMotion #334
base: master
Are you sure you want to change the base?
reduceMotion #334
Conversation
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 for the PR. Can you please refactor to check the preference once in the constructor, if the option is set? Then we will simply set this.motionOk
based on the check, and check motionOk
in the start method call.
dispatchEvent: jest.fn(), | ||
})), | ||
}); | ||
}); |
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.
Can you go ahead and add a test for the new option
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.
I don't have bandwidth at the moment, if anyone else does, please go for it.
src/countUp.ts
Outdated
@@ -21,6 +21,7 @@ export interface CountUpOptions { // (default) | |||
onCompleteCallback?: () => any; // gets called when animation completes | |||
onStartCallback?: () => any; // gets called when animation starts | |||
plugin?: CountUpPlugin; // for alternate animations | |||
reduceMotion?: boolean | 'auto'; // 'auto' respects user preference |
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.
I think the property name is misleading, let's change to checkReducedMotionPref
. The value should simply be a boolean, defaulting to 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.
How about respectPrefersReducedMotion
?
(boolean
, like you suggested, I dropped 'auto'
option, now true
is what 'auto'
was)
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.
Please use what I suggested
Sure! On my phone right now, I can do that soon. The reason I did it the way I did: it will re-check as needed, so if someone lands on a site and decides there's too much motion, they can toggle the setting on their device, and see a change without reloading the website. Note that it isn't checking mid-animation. |
I'm submitting a...
Checklist
Description
Fixes #327.
Basic implementation of respecting user's preference for reduced motion.
I would mark this as "draft" if the option was presented, as it is missing test coverage.
Does this PR introduce a breaking change?
Sort of, but the default value of
this.options.reduceMotion
could be changed tofalse
(rather than'auto'
). Kinda think it's better to just break the expectation that this plugin will default to ignoring the user's preference for reduced motion....