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

fix(sidenav): animate content resizing for side mode. #2486

Merged
merged 2 commits into from
Feb 4, 2017

Conversation

devversion
Copy link
Member

@devversion devversion commented Jan 1, 2017

  • Animates the margin-left and margin-right for the sidenav-content because otherwise when using mode="side" the sidenav content will just jump and not animate as same as the push mode.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 1, 2017
@devversion devversion changed the title fix(sidenav): animate content resizing for side mode. fix(sidenav): animate content resizing for side mode. Jan 1, 2017
@jelbourn
Copy link
Member

jelbourn commented Jan 3, 2017

cc @mmalerba

@devversion we had this before and it was super janky on mobile.

@devversion
Copy link
Member Author

devversion commented Jan 3, 2017

@jelbourn Hm, on the other hand just letting the content jump would probably also feel janky on all platforms

@mmalerba
Copy link
Contributor

mmalerba commented Jan 4, 2017

https://material.io/guidelines/patterns/navigation-drawer.html#navigation-drawer-behavior
The spec says that persistent drawers (equivalent of our "side" mode) should only be used on larger devices and that mobile should use temporary drawers (equivalent of our "over" mode).

I vote for adding the animation and adding a note to our docs that in addition to being the recommendation of the spec, we also suggest that they only use "over" or "push" on mobile devices for performance reasons

@devversion
Copy link
Member Author

@mmalerba I think that is a great idea. Do you want to me to include it into the OVERVIEW.md file?

@mmalerba
Copy link
Contributor

mmalerba commented Jan 4, 2017

yeah, sounds good

@devversion
Copy link
Member Author

@mmalerba Just added the info to the overview file.

@mmalerba
Copy link
Contributor

mmalerba commented Jan 5, 2017

@jelbourn is this ok with you?

@devversion
Copy link
Member Author

Ping @jelbourn for review.

@willshowell
Copy link
Contributor

I'm looking forward to this PR landing. Is there any progress on unblocking it?

@mmalerba mmalerba self-requested a review February 1, 2017 20:12
@mmalerba
Copy link
Contributor

mmalerba commented Feb 1, 2017

jeremy told me he wanted to test this on mobile, I just did and it looks fine to me. and again ideally people should use over or push on mobile.

* Animates the `margin-left` and `margin-right` for the `sidenav-content` because otherwise when using `mode="side"` the sidenav content will just jump and not animate as same as the `push` mode.
@devversion
Copy link
Member Author

@mmalerba Thanks for testing it on mobile.

@devversion devversion added action: merge The PR is ready for merge by the caretaker and removed blocked labels Feb 1, 2017
@kara kara merged commit 4d33449 into angular:master Feb 4, 2017
@devversion devversion deleted the fix/sidenav-side-transitions branch February 4, 2017 10:33
@andrewseguin
Copy link
Contributor

Sidenavs that initialize with opened are showing the content transition in. Using #3043 to track the issue

@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 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants