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

[EnhancedButton] remove unnecessary hack, improving overall performance #5868

Merged
merged 3 commits into from
Jan 4, 2017

Conversation

jampy
Copy link

@jampy jampy commented Jan 3, 2017

fixes #5867

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

This PR simply removes an older workaround that added a transform: translate(0,0) to all buttons. This forced the browser to create a GPU context for each button, preventing optimization of parent elements in some cases.

Since EnhancedButton is used in lots of other components, this PR perhaps improves overall performance in complex apps (but help of others is needed to verify this).

Fopr more information please see:

@muibot muibot added PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Jan 3, 2017
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

💯

@oliviertassinari oliviertassinari added PR: Review Accepted and removed PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Jan 4, 2017
@oliviertassinari oliviertassinari merged commit fd27433 into mui:master Jan 4, 2017
@oliviertassinari
Copy link
Member

@jampy Thanks for this PR! I agree with your points. Creating an additional layer is a tradeoff that should be well motivated. In our case, that wasn't providing any frame rate improvement, we were just paying the memory cost of them.

@oliviertassinari oliviertassinari mentioned this pull request Jan 7, 2017
3 tasks
@oliviertassinari oliviertassinari changed the title [EnhancedButton] remove old (now unnecessary) workaround, improving overall performance [EnhancedButton] remove unnecessary hack, improving overall performance Jan 15, 2017
* See: http://stackoverflow.com/questions/17298739/
* css-overflow-hidden-not-working-in-chrome-when-parent-has-border-radius-and-chil
*/
transform: disableTouchRipple && disableFocusRipple ? null : 'translate(0, 0)',

Choose a reason for hiding this comment

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

deleting this hack broke our design

Copy link
Member

Choose a reason for hiding this comment

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

Could you provide more detail?

Choose a reason for hiding this comment

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

@oliviertassinari we use FloatingActionButton with FontIcon that have rotate on hover, (in this case border radius not working)

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

Successfully merging this pull request may close these issues.

None yet

4 participants