-
Notifications
You must be signed in to change notification settings - Fork 55
chore(Animation): move to function component, styles in theme are not required #2258
Conversation
@layershifter is this related to #2247? |
@jurokapsiar Yes, it will fix it 👍 |
0c8cd09
to
af2af43
Compare
disableAnimations: context.disableAnimations, | ||
renderer: context.renderer, | ||
rtl: context.rtl, | ||
saveDebug: _.noop, |
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.
Should we have debug
for Animation
component?
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.
No 👍
Perf comparison
Potential regressions comparing to master
Perf tests with no regressions
Generated by 🚫 dangerJS |
(React.Children.only(children) as React.ReactElement) | ||
|
||
const element = ( | ||
<> |
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.
Remove <>
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.
Good catch, done 👍
rtl: context.rtl, | ||
saveDebug: _.noop, | ||
theme: context.theme, | ||
_internal_resolvedComponentVariables: context._internal_resolvedComponentVariables, |
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.
This is removed. Please update your branch
}) | ||
}, [ | ||
className, | ||
context, |
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.
Are we using everything from the context? Should we maybe list the things used from the context
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.
disableAnimations: context.disableAnimations,
renderer: context.renderer,
rtl: context.rtl,
theme: context.theme,
Almost everything... So I don't see any benefits in destructing it...
…ithub.com/stardust-ui/react into chore/move-animation-to-fc � Conflicts: � CHANGELOG.md
BREAKING CHANGES
Utility styles from the
Animation
component where moved to the component itself. No upgrade path there as there should no be any custom styles forAnimation
component as custom animations are defined in the theme.Fixes #2247.
Also affect affects performance improvements as styles now be recomputed only on demand.