Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

fix(components): remove animation prop from all components #2239

Merged
merged 6 commits into from
Jan 17, 2020

Conversation

joschect
Copy link
Contributor

Breaking Changes

  1. Animation property is being removed. Any component that makes use of the animation prop will break at build time. Any such components can be fixed by wrapping them in the existing <Animation/> component with the same animation props passed in.

Before

<Icon
    name="emoji"
    animation={{ name: 'spinner', delay: '5s', duration: '2s' }}
    circular
/>

After

<Animation name="spinner" delay="5s" duration="2s">
    <Icon name="emoji" circular />
</Animation>

Why?

The animation prop added uneeded bulk to the api surface and would only be used in certain situations. Removing it will improve perf slightly, as it was adding another check to every single component render. Additionally a quick search has shown that this prop is not currently used and that the animation component is already favored.

@DustyTheBot
Copy link
Collaborator

DustyTheBot commented Jan 14, 2020

Perf comparison

Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🔧 Avatar.Fluent 0.56 0.41 1.37:1 2000 1120
🔧 Button.Fluent 1.23 0.15 8.2:1 1000 1228
🔧 Checkbox.Fluent 1.36 0.3 4.53:1 1000 1358
🔧 Dialog.Fluent 0.32 0.17 1.88:1 5000 1601
🔧 Dropdown.Fluent 3.38 0.39 8.67:1 1000 3383
🔧 Icon.Fluent 0.26 0.03 8.67:1 5000 1284
🔧 Image.Fluent 0.11 0.08 1.38:1 5000 554
🔧 Slider.Fluent 2.85 0.31 9.19:1 1000 2845
🦄 Text.Fluent 0.05 0.17 0.29:1 5000 267
🦄 Tooltip.Fluent 0.37 18.37 0.02:1 5000 1845

🔧 Needs work     🎯 On target     🦄 Amazing

Generated by 🚫 dangerJS

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.

Please check the issue with snapshots before merging

@layershifter
Copy link
Member

Is there something that blocks merging?

@joschect
Copy link
Contributor Author

@layershifter Given the bug in Animation, I figured I should investigate that first. I've been looking into it. I guess I don't need a fix atm but I think it might be good to have a better idea as to why. That does seem like it might be an unrelated pr though.

@layershifter layershifter changed the title Styles: Remove animation prop fix(components): remove animation prop from all components Jan 17, 2020
@layershifter layershifter added ready for merge 🧰 fix Introduces fix for broken behavior. labels Jan 17, 2020
@layershifter
Copy link
Member

@joschect We had an offline conversation and agreed that we will fix linked issue separately, so there are no blockers for this PR 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🧰 fix Introduces fix for broken behavior. ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants