feat(nav): new sidenav #300

amcdnl · 2017-10-19T15:09:01Z

Implements new sidenav.

willshowell

Very excited for this!

willshowell · 2017-10-20T12:33:15Z

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

  $background: map-get($theme, background);
  $foreground: map-get($theme, foreground);
-  $sidenav: '.docs-component-viewer-sidenav';
+  $is-dark-theme: map-get($theme, is-dark);
+  $nav-background-color: if($is-dark-theme, 0.2, 0.03);


$nav-background-opacity?

willshowell · 2017-10-20T12:43:54Z

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

+          </a>
+        </li>
+      </ul>
+      <hr *ngIf="docItems.getCategories((params | async)?.section).length !== (i + 1)" />


How about using last so you don't have to sub twice,

<nav *ngFor="let category of docItems.getCategories((params | async)?.section); let last = last;">  <hr *ngIf="!last" /> </nav>

willshowell · 2017-10-20T12:47:56Z

src/app/pages/component-viewer/component-viewer.scss

@@ -2,25 +2,20 @@

 app-component-viewer {
  font-weight: 400;
-  padding: 20px $content-padding-side 50px;
+  padding: 20px 50px 50px 20px;


Maybe add a comment? Better yet, I see 20px, 25px, 50px coming up a lot. Could they be added to _constants.scss?

I did a search and didn't find any other cases, can you point me to the other usages?

I may have said that prematurely. I'll take a look when I get a chance to run it locally. Or I'll try to add sass constants throughout the app in a later PR.

willshowell · 2017-10-20T12:51:51Z

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

+              private _router: Router) { }
+
+  ngOnInit() {
+    this._router.events.subscribe(() => this.setExpansions());


Won't this create a ton of never-completing subscriptions? How about,

ngOnInit() { this._router.events .switchMap(() => this.params) .takeUntil(this._onDestroy) .subscribe(p => this.setExpansions(p)); } ngOnDestroy() { this._onDestroy.next(); this._onDestroy.complete(); } setExpansions(p: Params) { // ... }

You're right. Thanks for the suggestion!!

willshowell · 2017-10-20T12:53:21Z

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

+
+.docs-component-sidenav-body-content {
+  display: flex;
+  min-height: calc(100vh - 264px);


What is 264? It looks based on $sidenav-width somehow.

264 is the footer + header nav height.

Oh yeah lol of course the min-height doesn't have anything to do with sidenav width. Maybe add it as a variable with a comment?

Comment for sure at the least

willshowell · 2017-10-20T14:03:58Z

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

+  setExpansions() {
+    this.params.subscribe(p => {
+      const categories = this.docItems.getCategories(p.section);
+      for (const category of categories) {


As a followup of a previous comment, this should be,

setExpansions(p: Params) { const categories = this.docItems.getCategories(p.section); for (...) { // ... } }

so that it doesn't create another subscription. That also means that ngOnInit should updated like,

ngOnInit() { this._router.events .switchMap(() => this.params) .takeUntil(this._onDestroy) .subscribe(p => this.setExpansions(p)); }

If there is a problem that _router.events doesn't fire immediately, you could do this,

ngOnInit() { this._router.events .startWith(null) .switchMap(() => this.params) .takeUntil(this._onDestroy) .subscribe(p => this.setExpansions(p)); }

Beautiful and done.

jelbourn

Should the page have a minimum height so that it doesn't shrink when all of the sections are collapsed?
Any thoughts on adding an expand-all?
The click target for the links in the nav should stretch the entire width of the container

jelbourn · 2017-10-21T00:14:30Z

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


+  .docs-component-viewer-nav-inner {


Is there a more descriptive name than inner, or a comment you could add to explain what the element is?

jelbourn · 2017-10-21T00:17:46Z

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

+<div class="docs-component-viewer-nav">
+  <div class="docs-component-viewer-nav-inner">
+    <nav *ngFor="let category of docItems.getCategories((params | async)?.section); let last = last;">
+      <h3 (click)="toggleExpand(category.id)">


For a11y, each of these sections needs to act like an expansion panel. This means that the thing you interact with should be a <button> (with the same aria attrs that expansion panel uses). We shouldn't need to use a real heading element here since it's not part of the document content (table of contents).

jelbourn · 2017-10-21T00:18:24Z

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

-    </nav>
+    <app-component-nav
+      [params]="params">
+    </app-component-nav>


This looks like it could fit on one line

jelbourn · 2017-10-21T00:19:05Z

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

-    <app-footer></app-footer>
+    <div class="docs-component-sidenav-inner-content">
+      <div class="docs-component-sidenav-body-content">
+        <app-component-nav 


Add a comment explaining the different structure based on screen size?

jelbourn · 2017-10-21T00:20:27Z

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

+  display: block;
+  overflow: hidden;
+  position: absolute;
+  top: 50px;


Comment for 50px?

jelbourn · 2017-10-21T00:20:42Z

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

+
+.docs-component-sidenav-body-content {
+  display: flex;
+  min-height: calc(100vh - 264px);


Comment for sure at the least

jelbourn · 2017-10-21T00:20:52Z

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

+@media (max-width: $small-breakpoint-width) {
+  // Add specific rule to prevent default rule conflict
+  .docs-component-viewer-sidenav-container .docs-component-viewer-sidenav {
+    top: 42px;


Comment for 42px?

jelbourn · 2017-10-21T00:24:26Z

src/app/shared/table-of-contents/table-of-contents.scss

@@ -1,12 +1,14 @@
 :host {
  font-size: 13px;
-  width: 15%;
+  width: 19%;


jelbourn · 2017-10-21T00:24:52Z

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

+    this._onDestroy.complete();
+  }
+
+  setExpansions(params: Params) {


JsDoc for methods?

amcdnl · 2017-10-23T16:23:31Z

Should the page have a minimum height so that it doesn't shrink when all of the sections are collapsed?

That is handled by the 100vh calc above.

Any thoughts on adding an expand-all?

I haven't seen any other pages on Google have this, open to it if you have an idea how to.

The click target for the links in the nav should stretch the entire width of the container.

done.

jelbourn · 2017-10-23T16:42:11Z

package.json

@@ -4,7 +4,7 @@
  "license": "MIT",
  "angular-cli": {},
  "scripts": {
-    "start": "npm run build-themes && ng serve --aot --sourcemaps=false",


--aot

Want to add another command like start-jit that is set up that way?

jelbourn · 2017-10-23T16:50:36Z

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

+<div class="docs-component-viewer-nav">
+  <div class="docs-component-viewer-nav-content">
+    <nav *ngFor="let category of docItems.getCategories((params | async)?.section); let last = last;">
+      <button (click)="toggleExpand(category.id)">


Add an aria-label like "Expand the {{category.name}} section of navigation links"

Add aria-controls pointing to the element that expands

Add aria-expanded

amcdnl · 2017-10-23T20:12:49Z

@jelbourn - done :)

jelbourn · 2017-10-28T00:19:39Z

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

+  <div class="docs-component-viewer-nav-content">
+    <nav *ngFor="let category of docItems.getCategories((params | async)?.section); let last = last;">
+      <button (click)="toggleExpand(category.id)"
+              [attr.aria-label]="'Expand the ' + category.name"


I tried this out with VoiceOver. The label would be easier to understand as:
category.name + ', section toggle

jelbourn

LGTM

This reverts commit f1ddeae.

jelbourn · 2017-10-30T17:29:58Z

@amcdnl can you re-create this PR? I reverted it b/c it conflicts with a change Miles was working on that will change the structure of the sidenav first

This reverts commit f1ddeae.

amcdnl · 2017-10-30T18:07:42Z

@mmalerba - let me know when your ready for me to cut a new PR.

mmalerba · 2017-10-30T21:57:20Z

@amcdnl my change is merged now

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

Done. See #310

feat(nav): new sidenav

544fba9

amcdnl self-assigned this Oct 19, 2017

amcdnl requested a review from jelbourn October 19, 2017 15:09

googlebot added the cla: yes label Oct 19, 2017

willshowell reviewed Oct 20, 2017

View reviewed changes

chore(nit): updates per feedback

1866432

willshowell reviewed Oct 20, 2017

View reviewed changes

chore(nit): pr feedback

add0fd3

jelbourn reviewed Oct 21, 2017

View reviewed changes

chore(nit): PR feedback

5d1f851

Merge branch 'master' into new-sidenav

8ce095c

jelbourn reviewed Oct 23, 2017

View reviewed changes

amcdnl added 2 commits October 23, 2017 11:53

chore(nit): add back aot

38a02a2

chore(nit): a11y feedback

bac53e8

jelbourn reviewed Oct 28, 2017

View reviewed changes

chore(nit): pr feedback

0af56d0

jelbourn approved these changes Oct 30, 2017

View reviewed changes

jelbourn merged commit f1ddeae into angular:master Oct 30, 2017

amcdnl deleted the new-sidenav branch October 30, 2017 17:26

jelbourn added a commit that referenced this pull request Oct 30, 2017

Revert "feat(nav): new sidenav UX (#300)"

efc27cd

This reverts commit f1ddeae.

amcdnl added a commit to amcdnl/material.angular.io that referenced this pull request Oct 30, 2017

feat(nav): new sidenav UX (angular#300)

e9165c1

amcdnl pushed a commit to amcdnl/material.angular.io that referenced this pull request Oct 30, 2017

Revert "feat(nav): new sidenav UX (angular#300)"

b889f42

This reverts commit f1ddeae.

amcdnl mentioned this pull request Nov 1, 2017

New sidenav #310

Merged

feat(nav): new sidenav #300

feat(nav): new sidenav #300

Uh oh!

Conversation

amcdnl commented Oct 19, 2017

Uh oh!

willshowell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willshowell Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcdnl commented Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcdnl commented Oct 23, 2017

Uh oh!

jelbourn Oct 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn commented Oct 30, 2017

Uh oh!

amcdnl commented Oct 30, 2017

Uh oh!

willshowell Oct 20, 2017 •

edited

Loading

amcdnl commented Oct 23, 2017 •

edited

Loading

jelbourn Oct 28, 2017 •

edited

Loading