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

Reconsider margin transition on sidenav #404

Closed
jadjoubran opened this issue May 8, 2016 · 16 comments · Fixed by #460
Closed

Reconsider margin transition on sidenav #404

jadjoubran opened this issue May 8, 2016 · 16 comments · Fixed by #460

Comments

@jadjoubran
Copy link

  • Do you want to request a feature or report a bug?
    Bug
  • What is the current behavior?
    Poor performance for sidenav toggle transition
  • **If the current behavior is a bug,
    @jelbourn's material2 demo and sidenav template show that transition is targeting margin-left.
  • What is the expected behavior?
    I would expect animating transform: translateX unless there's a reason that I missed
  • What is the motivation / use case for changing the behavior?
    Smoother toggle transition
  • Which version of Angular and Material, and which browser and OS does this issue affect?
    Material alpha 4
  • Other information
    I would love to fix this if you agree
    Thanks!
@sebakerckhof
Copy link

The sidenav already animates using transform: translateX. However, the main content is animated using margin-left, which changes the width of the container and can result in poor performance.
But using translate there would not have the same behavior. One does actually want the width to change.

I think a good middle ground is the way google inbox does it. They animate the side bar, but they don't animate the main content. So the side nav slides in using a transform, but the width of the main content does not animate, but just jumps back and forth.

@jelbourn
Copy link
Member

jelbourn commented May 9, 2016

@hansl The sidenav is only animating the content element's margin when in side mode, right?

@hansl
Copy link
Contributor

hansl commented May 9, 2016

Yes, but we still set the transition: margin... property on the content even if it's not in side mode.

@jelbourn
Copy link
Member

jelbourn commented May 9, 2016

Shouldn't make a difference as long as it's not actually changing the margin, though, right?

@hansl
Copy link
Contributor

hansl commented May 9, 2016

That was the rationale, but I don't remember how much research I did on that subject.

@jelbourn
Copy link
Member

jelbourn commented May 9, 2016

@jadjoubran The margin should only be animating in side mode where changing the size of the content is desired. If you observe this happening in over or push mode, let us know. For now it seems like everything is working as intended.

@jelbourn jelbourn closed this as completed May 9, 2016
@sebakerckhof
Copy link

sebakerckhof commented May 9, 2016

In the demo app the margin does change on the md-content element when the side nav opens.

It is indeed necessary that the margin changes in side mode. But I think @jadjoubran argues that animating the margin is cpu-heavy and therefore not desirable.
I argue that you could do like google inbox does it.

@jadjoubran
Copy link
Author

Yeah, what @sebakerckhof said

@jelbourn
Copy link
Member

jelbourn commented May 9, 2016

We took out some test devices and did find that the relayouts in the side mode were indeed somewhat janky. Going to poke around with it.

@jelbourn jelbourn reopened this May 9, 2016
@jelbourn jelbourn changed the title Performance of sidenav toggle transition Reconsider margin transition on sidenav May 9, 2016
@jadjoubran
Copy link
Author

@jelbourn sounds great! It's actually visible on my macbook pro
Anyway, ping me if I can help

@jimjamdev
Copy link

Have you looked at calc?

i.e width: calc(100vw - $sidebarWidthVar)

@sebakerckhof
Copy link

sebakerckhof commented May 13, 2016

It's about animation performance, so I don't see where calc would help here. + I think the components should be as composable / nestable as possible, so I think it should not rely on viewport units.

@jadjoubran
Copy link
Author

@jelbourn Thanks for the fix! But animating with left will still be slow :/
The browser is recalculating style & layout everytime you change the left and the animation won't be as smooth as when you animate with transform translate
Codepen
Add a few macbooks and see the performance hit.. and then change left: 200px to transform: translateX(200px)

I can submit a PR if you want 😄

@jelbourn
Copy link
Member

Ah, I didn't notice that it wasn't already using translateX for the content. @robertmesserle want to do a quick change?

@robertmesserle
Copy link
Contributor

@jelbourn Sure - going to use translate3d() since it's faster in most browsers.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants