Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

robertpenner
Copy link
Collaborator

@robertpenner robertpenner commented Dec 7, 2024

Previous Behavior

createSlideStyles generates CSS keyframes that are used in Popover, MenuPopover, and through them, about a dozen components.

export function createSlideStyles(mainAxis: number): GriffelStyle {
  const fadeIn = {
    from: {
      opacity: 0,
    },
    to: {
      opacity: 1,
    },
  };
  // slide keyframes
  ...

New Behavior

The opacity CSS keyframes are removed from createSlideStyles and replaced with a Fade motion component.

Testing Notes

  • Use DevTools Animations inspector to run animations in slow motion.
  • The enter transition should have a fade-in during the slide. Design chose the default variant of Fade.
  • When the OS animations toggle is off, the slide should not happen, only the fade (matching current behavior).

Future Work

  • There is currently no exit transition, as the popover disappears instantly, whereas Design would like it to fade out. But this PR is about refactoring the fade implementation without adding new motion.
  • The slide CSS keyframes can be migrated to the motion framework, after we create a Slide motion component.

Related Issue(s)

@robertpenner robertpenner self-assigned this Dec 7, 2024
Copy link

github-actions bot commented Dec 7, 2024

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
221.431 kB
64.063 kB
220.896 kB
64.065 kB
-535 B
2 B
react-components
react-components: entire library
1.163 MB
291.493 kB
1.163 MB
291.371 kB
-860 B
-122 B
react-menu
Menu (including children components)
152.974 kB
46.179 kB
156.757 kB
47.709 kB
3.783 kB
1.53 kB
react-menu
Menu (including selectable components)
155.655 kB
46.661 kB
159.438 kB
48.205 kB
3.783 kB
1.544 kB
react-popover
Popover
129.217 kB
40.377 kB
133.089 kB
41.808 kB
3.872 kB
1.431 kB
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-avatar
Avatar
49.303 kB
15.815 kB
react-avatar
AvatarGroup
20.106 kB
7.968 kB
react-avatar
AvatarGroupItem
63.447 kB
20.034 kB
react-breadcrumb
@fluentui/react-breadcrumb - package
114.291 kB
31.695 kB
react-checkbox
Checkbox
35.118 kB
12.077 kB
react-combobox
Combobox (including child components)
104.401 kB
34.205 kB
react-combobox
Dropdown (including child components)
105.025 kB
34.13 kB
react-components
react-components: Button, FluentProvider & webLightTheme
69.21 kB
20.174 kB
react-components
react-components: FluentProvider & webLightTheme
44.447 kB
14.59 kB
react-datepicker-compat
DatePicker Compat
224.187 kB
63.434 kB
react-dialog
Dialog (including children components)
100.276 kB
30.064 kB
react-field
Field
23.399 kB
8.898 kB
react-input
Input
28.014 kB
9.444 kB
react-list-preview
List
89.164 kB
26.599 kB
react-list-preview
ListItem
112.731 kB
33.432 kB
react-overflow
hooks only
12.808 kB
4.819 kB
react-persona
Persona
56.194 kB
17.695 kB
react-portal-compat
PortalCompatProvider
8.39 kB
2.64 kB
react-positioning
usePositioning
27.079 kB
9.71 kB
react-progress
ProgressBar
17.084 kB
6.891 kB
react-radio
Radio
32.672 kB
10.343 kB
react-radio
RadioGroup
15.762 kB
6.423 kB
react-select
Select
27.732 kB
10.124 kB
react-slider
Slider
37.52 kB
12.621 kB
react-spinbutton
SpinButton
34.839 kB
11.63 kB
react-swatch-picker
@fluentui/react-swatch-picker - package
105.086 kB
30.516 kB
react-switch
Switch
35.319 kB
11.314 kB
react-table
DataGrid
161.034 kB
45.71 kB
react-table
Table (Primitives only)
42.666 kB
13.854 kB
react-table
Table as DataGrid
131.869 kB
36.57 kB
react-table
Table (Selection only)
70.536 kB
19.999 kB
react-table
Table (Sort only)
69.179 kB
19.61 kB
react-tag-picker
@fluentui/react-tag-picker - package
184.105 kB
55.426 kB
react-tags
InteractionTag
15.199 kB
6.157 kB
react-tags
Tag
29.072 kB
9.55 kB
react-tags
TagGroup
82.719 kB
24.524 kB
react-teaching-popover
TeachingPopover
90.633 kB
27.604 kB
react-textarea
Textarea
26.572 kB
9.755 kB
react-timepicker-compat
TimePicker
107.39 kB
35.763 kB
react-tooltip
Tooltip
56.074 kB
19.675 kB
react-tree
FlatTree
145.321 kB
41.739 kB
react-tree
PersonaFlatTree
146.007 kB
41.852 kB
react-tree
PersonaTree
142.24 kB
40.67 kB
react-tree
Tree
141.552 kB
40.568 kB
🤖 This report was generated against 39feb05abbd0f8ac4ab4e6bc0b41e198e512b2fe

Copy link

github-actions bot commented Dec 7, 2024

Pull request demo site: URL

@robertpenner robertpenner force-pushed the refactor/popover-fade-component branch 4 times, most recently from eafff79 to edd4126 Compare December 9, 2024 13:32
@robertpenner robertpenner marked this pull request as ready for review December 9, 2024 13:37
@robertpenner robertpenner requested a review from a team as a code owner December 9, 2024 13:37
@robertpenner robertpenner force-pushed the refactor/popover-fade-component branch from edd4126 to 619b080 Compare December 9, 2024 13:56
@robertpenner robertpenner requested a review from ling1726 December 9, 2024 14:03
Copy link
Member

@layershifter layershifter left a 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}>
Copy link
Member

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Popover] Migrate createSlideStyles to Fade motion component
4 participants