-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat(motion): add extended support for reduced motion #33353
base: master
Are you sure you want to change the base?
feat(motion): add extended support for reduced motion #33353
Conversation
📊 Bundle size reportUnchanged fixtures
|
Pull request demo site: URL |
@@ -6,5 +6,6 @@ import * as React from 'react'; | |||
export type MotionBehaviourType = 'skip' | 'default'; |
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.
🕵🏾♀️ visual regressions to review in the fluentuiv9 Visual Regression Report
Avatar Converged 2 screenshots
Image Name | Diff(in Pixels) | Image Type |
---|---|---|
Avatar Converged.badgeMask.chromium.png | 2 | Changed |
Avatar Converged.Badge Mask RTL.chromium.png | 5 | Changed |
Drawer 3 screenshots
Image Name | Diff(in Pixels) | Image Type |
---|---|---|
Drawer.Full Overlay High Contrast.chromium.png | 2219 | Changed |
Drawer.Full Overlay Dark Mode.chromium.png | 4157 | Changed |
Drawer.Full Overlay RTL.chromium.png | 1167 | Changed |
2f16fd0
to
07ad3b3
Compare
07ad3b3
to
7649e34
Compare
* | ||
* @see https://developer.mozilla.org/en-US/docs/Web/CSS/@media/prefers-reduced-motion | ||
*/ | ||
reducedMotion?: { keyframes?: Keyframe[] } & KeyframeEffectOptions; |
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 to me that reducedMotion
is a recursion of AtomMotion
🤔, maybe it would be better to add it somewhere else to avoid repetition? or perhaps we should have a middle type here between PresenceMotion
and AtomMotion
?
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.
Maybe something similar to griffel fallback properties?!
const Motion = createPresenceComponent([
// default motion
{
enter: {
keyframes: [
{ opacity: 0, transform: 'scale(0)' },
{ opacity: 1, transform: 'scale(1)' },
],
duration: 300,
},
exit: {
/* ... */
},
},
{/* reduced motion goes here */}
]);
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.
@bsunderhus I also think a middle type would be good, to DRY up
{ keyframes: Keyframe[] } & KeyframeEffectOptions
.
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.
from @bsunderhus
or perhaps we should have a middle type here betweenPresenceMotion
andAtomMotion
?
from @robertpenner
I also think a middle type would be good, to DRY up
{ keyframes: Keyframe[] } & KeyframeEffectOptions
.
Do you mean a new type to extract { keyframes?: Keyframe[] } & KeyframeEffectOptions
?
Maybe something similar to griffel fallback properties?!
@bsunderhus I updated the PR description with two options and added some arguments against them. Please check and let me know WDYT.
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.
Do you mean a new type to extract { keyframes?: Keyframe[] } & KeyframeEffectOptions?
Is it necessary to make keyframes
optional in reducedMotion
(could we look at a use case)? Or would it be simpler to say, "If you're going to make a custom reduced motion, you need to define keyframes"?
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.
Is it necessary to make keyframes optional in reducedMotion (could we look at a use case)? Or would it be simpler to say, "If you're going to make a custom reduced motion, you need to define keyframes"?
I don't think that we need to force consumers there, especially after the duration change (#33353 (comment)). Also, the media query itself (https://developer.mozilla.org/en-US/docs/Web/CSS/@media/prefers-reduced-motion) does not put any restrictions.
The feature motivation is solid; the feature design needs to be fleshed out a bit more like an RFC (if it's not going to be an actual RFC). For example:
|
FYI, it's was mentioned in the original RFC as a proposal: https://github.com/microsoft/fluentui/blob/master/docs/react-v9/contributing/rfcs/react-components/convergence/motion-definition-n-apis.md#advanced-reduced-motion-support
I updated the PR description to list other two options, is there anything that comes to your mind? Did I miss something?
As the atom definition changes, it will work transparently. I added an example to the PR description. |
* | ||
* @see https://developer.mozilla.org/en-US/docs/Web/CSS/@media/prefers-reduced-motion | ||
*/ | ||
reducedMotion?: { keyframes?: Keyframe[] } & KeyframeEffectOptions; |
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.
Just throwing out another idea, instead of making the reduced motion options a recursion of the parent motion declaration we use a second argument in the factory and enforce the SRP for motion declarations?
createPresentionComponent(atom, reducedMotionAtom?)
This way we'll avoid TS recursion, and decouple the original motion and its reduced motion alternative. From what I understand even the current reducedMotion
property completely overrides all other options anyway
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's not recursion though; it's just leaf reuse. There is no atom inside an atom, nor keyframe inside a keyframe.
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.
@ling1726 please check "rejected options" in the PR's description, the order issue will be the same here.
...(isReducedMotion && { duration: 1 }), | ||
}); | ||
...(isReducedMotion && reducedMotionParams), |
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.
If a user has provided reducedMotion params, I think we should stop applying the default duration: 1
value.
It might sound stupid to pass no duration and it defaults to 0, but we should still the user have the full override power in this case
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.
If a user has provided reducedMotion params, I think we should stop applying the default duration: 1 value.
I wanted to avoid breaks, that's why I kept it. A user will be still able to override duration
, so I don't see any harm with the current behavior.
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.
@ling1726 @layershifter What if we forced them to override duration
for reducedMotion
? After all, if the duration stays at 1 ms, new keyframes aren't going to accomplish much, in most use cases. And there might be someone who adds reducedMotion
keyframes, then is puzzled when they don't seem to work, because they don't realize the duration is at 1 ms and they need to change it.
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.
@ling1726 @layershifter What if we forced them to override
duration
forreducedMotion
? After all, if the duration stays at 1 ms, new keyframes aren't going to accomplish much, in most use cases. And there might be someone who addsreducedMotion
keyframes, then is puzzled when they don't seem to work, because they don't realize the duration is at 1 ms and they need to change it.
That's valid concern. As both you (@robertpenner) and @ling1726 raised it, I updated the implementation:
reducedMotion
isundefined
=>{ duration: 1ms }
reducedMotion
is defined => we will use whatever user provided
@@ -66,6 +66,10 @@ const FadeEnter = createMotionComponent({ | |||
keyframes: [{ opacity: 0 }, { opacity: 1 }], | |||
duration: motionTokens.durationSlow, | |||
iterations: Infinity, | |||
|
|||
reducedMotion: { | |||
iterations: 1, |
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.
What's the value of having a single iteration here? Is it just that this property has a clearer presence in documentation?
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.
With infinite iterations, the reduced motion with 1 ms was looping and flashing.
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.
👆 Exactly, either this or we need to complicate examples significantly.
@layershifter I think you've covered it well.
Excellent additions, thanks. |
7649e34
to
7128361
Compare
Previous Behavior
No way to customize motions' behavior when reduced motion is enabled.
New Behavior
reducedMotion
option toAtomMotion
type.By default, when reduced motion is enabled the duration of the animation is set to
1ms
(current behavior).reducedMotion
allows to customize a reduced motion version of the animation:This will also works with arrays of atoms as
reducedMotion
definitions are collocated to its atoms:Rejected options
Second argument to
createMotionComponent()
&createPresenceComponent()
We have already an array syntax for multiple atoms (see createPresenceComponent() and arrays):
The proposal is two have a second argument:
However, it will be hard to distinguish between atoms and reduced motion declarations - you have to know the order of arguments in the functions to understand it.
This implementation also brings a challenge of collocation: reduced options should be optional, however we will need to maintain the order to make it work. The example below shows the requirement:
Consumers will be forced to pass
null
(or something else?) in this case:Nest atoms
That's very similar to previous option, but instead we will have syntax similar to fallback values in Griffel.
In this case we collocate reduced options with the atom they are related to, so we don't need to maintain the order like in the previous option 👍
However, this option has the same issue with being implicit and hard to understand: which array stands for atoms and which for reduced options?
Related issues
Fixes #33358.