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(Collapse): streamline atom functions #33463

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

Conversation

robertpenner
Copy link
Collaborator

@robertpenner robertpenner commented Dec 12, 2024

Previous Behavior

  • Duplication in enter/exit implementations in atom functions:
    • opacity
    • whitespace
  • No fade atom function in common location

New Behavior

  • Duplication factored out
  • fadeAtom is in a common location; Collapse and Fade import it for reuse
  • Comments and explanatory types are added as needed

Future Work

  • Use fadeAtom in other motion components wherever possible

Related Issue(s)

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

github-actions bot commented Dec 12, 2024

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-accordion
Accordion (including children components)
107.106 kB
32.792 kB
106.948 kB
32.803 kB
-158 B
11 B
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
222.729 kB
64.426 kB
222.571 kB
64.436 kB
-158 B
10 B
react-components
react-components: entire library
1.164 MB
291.241 kB
1.164 MB
291.25 kB
-187 B
9 B
react-toast
Toast (including Toaster)
101.397 kB
30.485 kB
101.239 kB
30.481 kB
-158 B
-4 B
react-tree
FlatTree
145.488 kB
41.779 kB
145.328 kB
41.787 kB
-160 B
8 B
react-tree
PersonaFlatTree
146.176 kB
41.89 kB
146.016 kB
41.898 kB
-160 B
8 B
react-tree
PersonaTree
142.407 kB
40.711 kB
142.248 kB
40.717 kB
-159 B
6 B
react-tree
Tree
141.719 kB
40.611 kB
141.56 kB
40.614 kB
-159 B
3 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-components
react-components: Button, FluentProvider & webLightTheme
69.236 kB
20.182 kB
react-components
react-components: FluentProvider & webLightTheme
44.473 kB
14.597 kB
react-dialog
Dialog (including children components)
100.443 kB
30.105 kB
react-message-bar
MessageBar (all components)
24.851 kB
9.276 kB
react-portal-compat
PortalCompatProvider
8.39 kB
2.64 kB
react-timepicker-compat
TimePicker
108.551 kB
36.094 kB
🤖 This report was generated against 8cf401d626def27ad679f9e53928533df9f9ef52

Copy link

Pull request demo site: URL

@robertpenner robertpenner force-pushed the refactor/collapse-atoms branch from 0aea9d7 to 3608e82 Compare December 12, 2024 22:49
@robertpenner robertpenner force-pushed the refactor/collapse-atoms branch 6 times, most recently from 53ccc82 to fca343d Compare December 18, 2024 18:17
@robertpenner robertpenner force-pushed the refactor/collapse-atoms branch from fca343d to 6eabc48 Compare January 13, 2025 19:42
@robertpenner robertpenner force-pushed the refactor/collapse-atoms branch 3 times, most recently from 2fdcbee to ede82f1 Compare January 13, 2025 20:09
@robertpenner robertpenner marked this pull request as ready for review January 13, 2025 20:10
@robertpenner robertpenner requested a review from a team as a code owner January 13, 2025 20:10
@robertpenner robertpenner force-pushed the refactor/collapse-atoms branch from ede82f1 to acd854e Compare January 14, 2025 13:55
@pixel-perfectionist
Copy link
Member

The PR description mentions that fadeAtom is in a common location and is imported into Collapse for reuse. Should the same logic be applied to the white space and container size atoms? Would it make sense to refactor these as reusable basic instances and move them to a shared location for consistency?

@pixel-perfectionist
Copy link
Member

I noticed the interfaces SizeEnterAtomParams, SizeExitAtomParams, and WhitespaceAtomParams share overlapping properties such as duration, easing, and delay. To improve consistency and reusability, could we introduce a global reusable motion atom interface that these interfaces could extend? This would reduce redundancy and make the motion atom parameters more maintainable. For example:

interface BaseMotionAtomParams {
  duration: number;
  easing: string;
  delay?: number;
}

interface SizeEnterAtomParams extends BaseMotionAtomParams {
  element: HTMLElement;
  fromSize?: string;
}

@pixel-perfectionist
Copy link
Member

delay property is currently optional and is only included in some interfaces SizeExitAtomParams and WhitespaceAtomParams. Shouldn’t delay be consistently available for any motion atom?

@robertpenner
Copy link
Collaborator Author

The PR description mentions that fadeAtom is in a common location and is imported into Collapse for reuse. Should the same logic be applied to the white space and container size atoms? Would it make sense to refactor these as reusable basic instances and move them to a shared location for consistency?

Good question. I moved fadeAtom to a common location because the abstraction is clear, simple and reusable in multiple motion components already. (I just decided to refactor Fade to use fadeAtom in 1f75471.)

Whereas the whitespace and container size atoms have only been used in Collapse, and we don't have plans to use them in other motion components in the short term. Since the abstraction has not been tested across multiple components, it seems best to leave them as collapse-specific atoms, until such time as we need to reuse them elsewhere.

@robertpenner
Copy link
Collaborator Author

delay property is currently optional and is only included in some interfaces SizeExitAtomParams and WhitespaceAtomParams. Shouldn’t delay be consistently available for any motion atom?

This is an interesting abstraction consideration that I'm still pondering. With fadeAtom, it seemed better to omit delay and fill from the atom signature, and instead add those properties at the place they're needed:

enterAtoms.push({
  ...fadeAtom({ direction: 'enter', duration: enterOpacityDuration, easing: enterEasing }),
  delay: enterDelay,
  fill: 'both',
});

Of course, we could allow fadeAtom to receive extra properties like these, But then it's just spitting out the exact thing you're putting into it; the fadeAtom function doesn't do any useful abstraction around delay or fill.

I chose a different approach for sizeExitAtom, making delay a function parameter, because Collapse has a delayed variant where the size exit must be delayed; it's essential to the motion sequencing. But I am thinking about whether to make it consistent with fadeAtom and remove delay from the atom function, since sizeExitAtom also just spits out delay unchanged.

@robertpenner
Copy link
Collaborator Author

I noticed the interfaces SizeEnterAtomParams, SizeExitAtomParams, and WhitespaceAtomParams share overlapping properties such as duration, easing, and delay. To improve consistency and reusability, could we introduce a global reusable motion atom interface that these interfaces could extend? This would reduce redundancy and make the motion atom parameters more maintainable. For example:

Sure, we're at the point where duration and easing are showing up in multiple types; it makes sense to DRY them up. As for delay, I don't see that as needing to be added everywhere currently (see my other comment).

@pixel-perfectionist
Copy link
Member

delay property is currently optional and is only included in some interfaces SizeExitAtomParams and WhitespaceAtomParams. Shouldn’t delay be consistently available for any motion atom?

This is an interesting abstraction consideration that I'm still pondering. With fadeAtom, it seemed better to omit delay and fill from the atom signature, and instead add those properties at the place they're needed:

enterAtoms.push({
  ...fadeAtom({ direction: 'enter', duration: enterOpacityDuration, easing: enterEasing }),
  delay: enterDelay,
  fill: 'both',
});

Of course, we could allow fadeAtom to receive extra properties like these, But then it's just spitting out the exact thing you're putting into it; the fadeAtom function doesn't do any useful abstraction around delay or fill.

I chose a different approach for sizeExitAtom, making delay a function parameter, because Collapse has a delayed variant where the size exit must be delayed; it's essential to the motion sequencing. But I am thinking about whether to make it consistent with fadeAtom and remove delay from the atom function, since sizeExitAtom also just spits out delay unchanged.

Thank you for the clarification! While I understand the current scope of white space and container size atoms being specific to the Collapse component, I believe establishing a consistent system for how we structure and build motion components would be beneficial in the long term.

Having a shared space for reusable atoms, even if they’re currently scoped to a single component, would make the system easier to extend and maintain. It would also prevent the loss of context for future developers unfamiliar with the current setup. Maintaining atomic logic where atoms are reusable building blocks ensures clarity and consistency across the project.

When atoms are scoped exclusively to one molecule (like Collapse in this case), it deviates from the principle of reusable design systems and might make the logic harder to track or reuse later. A system-first approach, where atoms are abstracted into reusable components, aligns better with best practices.

@robertpenner
Copy link
Collaborator Author

robertpenner commented Jan 14, 2025

When atoms are scoped exclusively to one molecule (like Collapse in this case), it deviates from the principle of reusable design systems and might make the logic harder to track or reuse later. A system-first approach, where atoms are abstracted into reusable components, aligns better with best practices.

Reusable abstractions are good, when we've tested the abstraction through reuse across differing use cases. But if we haven't reused the abstraction, we haven't actually tested it well enough to know if it's ready to reuse. Premature Abstraction is a real problem that we must be mindful of. At this point, it seems premature to move atoms into the common space before we validate their reusability in multiple usage scenarios.

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.

[Collapse] Streamline motion atom functions
3 participants