-
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
refactor(Popover): migrate opacity transition to Fade component #33424
base: master
Are you sure you want to change the base?
refactor(Popover): migrate opacity transition to Fade component #33424
Conversation
📊 Bundle size reportUnchanged fixtures
|
Pull request demo site: URL |
change/@fluentui-react-popover-fec81255-7a72-4ab1-9324-c96c3961e502.json
Outdated
Show resolved
Hide resolved
eafff79
to
edd4126
Compare
edd4126
to
619b080
Compare
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.
Let's avoid one migration and migrate the "slide" part, too. There is no sense to keep one animation in CSS and another in JS.
{state.withArrow && <div ref={state.arrowRef} className={state.arrowClassName} />} | ||
{state.root.children} | ||
</state.root> | ||
<Fade visible={state.open}> |
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.
We should introduce a motion slot if we add an animation, otherwise there is no way to remove it/customize 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.
We're not adding any animation; we're just swapping the implementation of what was already there.
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.
We've seen migration to react-motion
that doesn't add a motion slot:
Previous Behavior
createSlideStyles
generates CSS keyframes that are used inPopover
,MenuPopover
, and through them, about a dozen components.New Behavior
The opacity CSS keyframes are removed from
createSlideStyles
and replaced with aFade
motion component.Testing Notes
Fade
.Future Work
Slide
motion component.Related Issue(s)
createSlideStyles
toFade
motion component #33422