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

feat(sidenav): Add support for fixed sidenavs #6712

Merged
merged 11 commits into from
Sep 21, 2017

Conversation

mmalerba
Copy link
Contributor

@mmalerba mmalerba commented Aug 29, 2017

temporary demo: https://mmalerba-demo5.firebaseapp.com/sidenav

fixes #3031
fixes #1515
fixes #1179
fixes #998
fixes #778

@mmalerba mmalerba requested a review from jelbourn August 29, 2017 23:44
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 29, 2017
@@ -22,7 +22,7 @@ import {
NgZone,
OnDestroy,
Inject,
ChangeDetectorRef,
ChangeDetectorRef, ContentChild, forwardRef,
Copy link
Member

Choose a reason for hiding this comment

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

Reformat?

})
export class MdDrawerContent implements AfterContentInit {
/** Margins to be applied to the content. */
_margins: {left: number, right: number} = {left: 0, right: 0};
Copy link
Member

Choose a reason for hiding this comment

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

Why are these margins here? Add explanation for why this is a thing?

}

ngAfterContentInit() {
this._container._contentMargins.subscribe((margins) => {
Copy link
Member

Choose a reason for hiding this comment

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

nit: don't need parents around margins

@@ -155,6 +193,8 @@ export class MdDrawer implements AfterContentInit, OnDestroy {
/** @deprecated */
@Output('align-changed') onAlignChanged = new EventEmitter<void>();

_modeChanged = new Subject();
Copy link
Member

Choose a reason for hiding this comment

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

Add comment explaining why this is a thing?

private _updateContentMargins() {
let left = 0;
let right = 0;

Copy link
Member

Choose a reason for hiding this comment

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

Add comment that describes which style is applied where based on the mode + position?

export class MdSidenav extends MdDrawer {}
export class MdSidenav extends MdDrawer {
/** Whether the sidenav is fixed in the viewport. */
@Input() fixedInViewport = true;
Copy link
Member

Choose a reason for hiding this comment

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

Use coerceBooleanProperty

* The gap between the top of the sidenav and the top of the viewport when the sidenav is in fixed
* mode.
*/
@Input() fixedTopGap = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Use coerceNumberProperty

export class MdSidenav extends MdDrawer {}
export class MdSidenav extends MdDrawer {
/** Whether the sidenav is fixed in the viewport. */
@Input() fixedInViewport = true;
Copy link
Member

Choose a reason for hiding this comment

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

Add unit tests for these new fixed properties?

@mmalerba
Copy link
Contributor Author

mmalerba commented Sep 2, 2017

@jelbourn comments addressed

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Sep 5, 2017
@mmalerba mmalerba force-pushed the snav-fixed2 branch 3 times, most recently from 859fc7b to ff9307f Compare September 11, 2017 22:20
@mmalerba mmalerba added the P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful label Sep 13, 2017
@mmalerba mmalerba force-pushed the snav-fixed2 branch 8 times, most recently from c491cfe to bf1cc46 Compare September 20, 2017 23:05
@axtho
Copy link

axtho commented Sep 27, 2017

Can we get this into the material2-builds?

@kaykhan
Copy link

kaykhan commented Oct 5, 2017

Do you have the source code for the working version in the demo?

@kenji-1996
Copy link

@KKHAN1991 I looked into the css of the working example

.footer_fixed {
  display: flex;
  box-sizing: border-box;
  width: 100%;
  padding: 0 16px;
  flex-direction: column;
}

and then apply this to your footer item like so:

<mat-toolbar class="footer_fixed">
  <div class="container">
    <span>this is a toolbar</span>
  </div>
</mat-toolbar>

Then I just call my app-footer (i made a component for this) inside my sidebar nav and below my navbar like so;

<mat-sidenav-container fullscreen>
  <mat-sidenav mode="push" #sidenav>
    <div fxLayout="column">
      .. sidenav content ..
  </mat-sidenav>

  <mat-toolbar color="primary" class="fixed-navbar mat-elevation-z10">
    .. material toolbar content ..
  </mat-toolbar>

  ..container for inside elements
  <app-container></app-container>
  <footer>
    .. my footer element inside footer tags ..
    <app-footer></app-footer>
  </footer>

</mat-sidenav-container>

Hope this helps, ive spent the past 4 hours fixing problems and getting it working so i hope i can save someone time

@axtho
Copy link

axtho commented Oct 13, 2017

@KKHAN1991 here is the demo: https://material2-dev.firebaseapp.com/sidenav, and here are the files that produce it: https://github.com/angular/material2/tree/master/src/demo-app/sidenav. It works well.

@Splaktar
Copy link
Member

Is there any documentation for this feature other than just @Input() fixedInViewport boolean | Whether the sidenav is fixed in the viewport. in the API section of the sidenav docs?

@mmalerba
Copy link
Contributor Author

@Splaktar soon #7775

@mmalerba mmalerba deleted the snav-fixed2 branch April 3, 2018 15:19
@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 8, 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 P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful
Projects
None yet
7 participants