New sidenav #310

amcdnl · 2017-11-01T15:41:15Z

This PR implements the new left nav. This is the exact same PR as #300 just merged w/ @mmalerba latest changes.

mmalerba · 2017-11-01T16:19:59Z

I see a couple of issues here:

In mobile mode, the document does not receive scroll events (this is needed for the address bar to scroll up on phones (compare to https://material2-docs-dev.firebaseapp.com/components/autocomplete/overview where the document does receive scroll events)
In mobile mode, there seems to be a gap between the 2 toolbars
In desktop mode, there is a gap at the bottom (below the footer)
In desktop mode, when I expand the sidenav both the content and the sidenav scroll (the content doesn't actually need to scroll, it's only scrolling because of the sidenav)

… into new-sidenav2

amcdnl · 2017-11-01T19:04:15Z

So that is tricky because of the sticky positioned items. I was able to work around this by adding a hack that will automatically hide the browser address bar for you.
Fixed
Fixed
I'm not sure its possible to fix this given the nature of things that need to scroll / push in this scenario. Open to suggestions though.

amcdnl · 2017-12-11T18:49:45Z

tag @jelbourn

jelbourn · 2017-12-12T17:09:15Z

@mmalerba can you review this?

mmalerba · 2017-12-12T19:37:37Z

Sorry, I thought this wasn't ready since it has conflicts. @amcdnl Can you resolve it so I can bring up a demo?

# Conflicts: # src/app/pages/component-sidenav/component-sidenav.ts # src/app/shared/table-of-contents/table-of-contents.scss # src/index.html

amcdnl · 2018-01-21T17:26:34Z

@mmalerba - merged :)

jelbourn · 2018-01-22T17:18:35Z

@mmalerba you can merge this directly once it has your approval

mmalerba · 2018-01-22T17:46:59Z

@amcdnl could you take a stab at trying to fix the mobile scrolling and the slightly too tall page? I talked to Jeremy about whether its worth the trade-off for a prettier sidenav, and we'd really prefer not to lose these features (especially the mobile scrolling)

code-wise looks good minus the couple newline nits

amcdnl · 2018-02-04T17:32:10Z

@mmalerba - I believe I had addressed all your feedback and am ready for review! :)

mmalerba · 2018-02-05T17:22:44Z

src/app/pages/component-sidenav/_component-sidenav-theme.scss


+  .docs-component-viewer-nav-content {
+    background: rgba(0, 0, 0, $nav-background-opacity);


should this be the foreground color instead of always black?

I wanted to make it a slight lighter accent of the current background. This is mainly done like this for different themes.

mmalerba · 2018-02-05T17:22:56Z

src/app/pages/component-sidenav/_component-sidenav-theme.scss


-  .docs-component-viewer-sidenav {
+    &::-webkit-scrollbar-thumb {
+      background: rgba(0, 0, 0, .26);


same comment as above

mmalerba · 2018-02-05T17:23:12Z

src/app/pages/component-sidenav/_component-sidenav-theme.scss

+
+    .mat-drawer {
+      &::-webkit-scrollbar-thumb {
+        background: rgba(0, 0, 0, .26);


same comment as above

mmalerba · 2018-02-05T17:23:30Z

src/app/pages/component-sidenav/component-nav.html

+      <hr *ngIf="!last" />
+    </nav>
+  </div>
+</div>


mmalerba · 2018-02-05T17:23:45Z

src/app/pages/component-sidenav/component-sidenav.html

  </div>
-</mat-sidenav-container>
+</mat-sidenav-container>


mmalerba · 2018-02-05T17:31:03Z

Awesome, it looks much better now. It looks like this one is still an issue:

In desktop mode, when I expand the sidenav both the content and the sidenav scroll (the content doesn't actually need to scroll, it's only scrolling because of the sidenav)

I don't think it's a huge deal, I would be ok submitting the PR if it's too difficult to fix, but it would be nice to fix.

amcdnl · 2018-02-05T18:49:34Z

@mmalerba - The nav is always a fixed 75vh so it should be scrolling in this case. The body scrolling in this case is unexpected and I thought I had fixed this. Let me take another look and let you know.

amcdnl · 2018-02-05T23:10:24Z

@mmalerba - Really the only good fix for this would be to remove the scrollbar from the side nav and just let it push down. I implemented the scrollbar like this similar to the google api docs that I used as an example. Let me know how you would like to proceed.

mmalerba · 2018-02-05T23:52:13Z

we can't do something like this?

max-height: calc(100vh - <header-height> - <footer-height>)

amcdnl · 2018-02-06T19:09:46Z

amcdnl added 2 commits November 1, 2017 10:40

feat(nav): implement new left nav

c9b1f5b

feat(nav): cont imp left nav

5c06dd4

amcdnl requested a review from jelbourn November 1, 2017 15:41

googlebot added the cla: yes label Nov 1, 2017

amcdnl mentioned this pull request Nov 1, 2017

feat(nav): new sidenav #300

Merged

amcdnl added 2 commits November 1, 2017 13:07

Merge branch 'master' of https://github.com/angular/material.angular.io…

f5d9098

… into new-sidenav2

chore(nit): address PR feedback

ba28d51

jelbourn requested a review from mmalerba December 12, 2017 17:09

Merge branch 'master' into new-sidenav2

9fd6d13

# Conflicts: # src/app/pages/component-sidenav/component-sidenav.ts # src/app/shared/table-of-contents/table-of-contents.scss # src/index.html

amcdnl added 3 commits February 4, 2018 09:46

Merge branch 'master' into new-sidenav2

45b6b23

fix: window scroll event not working

b2cfa71

fix: footer spacing causing scrollbar overflows

566fe51

mmalerba reviewed Feb 5, 2018

View reviewed changes

chore: nit feedback

6b880bb

mmalerba merged commit 81e8698 into angular:master Feb 6, 2018

amcdnl deleted the new-sidenav2 branch February 6, 2018 19:08


		.docs-component-viewer-nav-content {
		background: rgba(0, 0, 0, $nav-background-opacity);

New sidenav #310

New sidenav #310

Uh oh!

Conversation

amcdnl commented Nov 1, 2017

Uh oh!

mmalerba commented Nov 1, 2017

Uh oh!

amcdnl commented Nov 1, 2017

Uh oh!

amcdnl commented Dec 11, 2017

Uh oh!

jelbourn commented Dec 12, 2017

Uh oh!

mmalerba commented Dec 12, 2017

Uh oh!

amcdnl commented Jan 21, 2018

Uh oh!

jelbourn commented Jan 22, 2018

Uh oh!

mmalerba commented Jan 22, 2018

Uh oh!

amcdnl commented Feb 4, 2018

Uh oh!

mmalerba Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

amcdnl Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

mmalerba Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

mmalerba Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

mmalerba Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

mmalerba Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

mmalerba commented Feb 5, 2018

Uh oh!

amcdnl commented Feb 5, 2018

Uh oh!

amcdnl commented Feb 5, 2018

Uh oh!

mmalerba commented Feb 5, 2018

Uh oh!

amcdnl commented Feb 6, 2018

Uh oh!

Uh oh!