feat(toc): add table of contents to overview and guides #230

amcdnl · 2017-07-26T01:59:38Z

Addresses #167 by adding a table of contents and jump links to the component overview and guide pages.

This PR depends on angular/components#6040 being merged.

commit b684808 Author: Austin <[email protected]> Date: Mon Jul 17 15:35:08 2017 -0600 feat(PWA): add service worker angular#175 (angular#223) commit ec7c637 Author: Austin <[email protected]> Date: Mon Jul 17 13:52:28 2017 -0600 feat(docs): add page titles to doc pages angular#189 (angular#220) commit 6170057 Author: Rafael Santana <[email protected]> Date: Wed Jul 12 20:05:29 2017 -0300 Fix exception being thrown on invalid routes (angular#218) commit 101aeff Author: Michael Prentice <[email protected]> Date: Tue Jul 11 19:38:17 2017 -0400 fix(footer): extra w in www.angular.io (angular#217) just use https://angular.io since that is where www.angular.io goes commit 135c38f Author: Rafael Santana <[email protected]> Date: Sun Jul 9 12:37:39 2017 -0300 Add handling for invalid routes (angular#191) commit ad1400e Author: Will Howell <[email protected]> Date: Sun Jul 9 11:35:25 2017 -0400 Set max width for component and guide viewers (angular#207) commit ac3f30e Author: Austin <[email protected]> Date: Sun Jul 9 10:18:02 2017 -0500 chore(styles): apply material light / dark styles to highlightjs (angular#213)

jelbourn · 2017-07-26T17:39:48Z

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

+  }
+
+  a {
+    color: mat-color($foreground, secondary-text);


This is too broad; it's affecting every anchor in the app, including the top nav and examples. Should be scoped to a css class

jelbourn · 2017-07-26T17:40:11Z

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

+  $background: map-get($theme, background);
+  $foreground: map-get($theme, foreground);
+
+  .toc-container {


Prefix docs styles with docs-

jelbourn · 2017-07-26T17:41:22Z

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

  min-height: 500px;
+
+  table-of-contents {


Does this need to be nested? Nesting for organization should be avoided
https://github.com/angular/material2/blob/master/CODING_STANDARDS.md#use-lowest-specificity-possible

Yes, it needs to override the host top position.

jelbourn · 2017-07-26T17:43:48Z

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

+  flex-grow: 1;
+}
+
+.component-overview {


Prefix with docs-

jelbourn · 2017-07-26T17:45:05Z

src/app/pages/guide-viewer/guide-viewer.html

@@ -3,7 +3,10 @@
 </div>

 <div class="docs-guide-wrapper">
-  <doc-viewer class="docs-guide-content" [documentUrl]="guide.document"></doc-viewer>
+  <div class="docs-guide-inner">


Is there a more specific name than inner? I find class names line this to be hard to understand when coming back to the code later. Better to be verbose with something like docs-guide-toc-and-content

jelbourn · 2017-07-26T18:30:10Z

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

+    this._scrollSubscription = Observable
+      .fromEvent(this._scrollContainer, 'scroll')
+      .debounceTime(10)
+      .subscribe(this.onScroll.bind(this));


Need to unsubscribe on destroy

jelbourn · 2017-07-26T18:50:31Z

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

+    }
+    return false;
+  }
+


return this.scrollOffset >= currentLink.top && !(nextLink && nextLink.top < this.scrollOffset);

Also with a comment like A link is considered "active" if the page is scrolled passed it without also being scrolled passed the following link

jelbourn · 2017-07-26T18:51:23Z

src/styles/_markdown-theme.scss

+  .docs-markdown-h4 {
+    position: relative;
+    padding-left: 30px;
+    margin-left: -30px;


Why the 30 / -30 here? At the very least needs a comment

Want the anchor to position 30px to the left of the header. Using absolutely position makes the anchor link not clickable because its outside the header hover.

Added comments for this.

jelbourn · 2017-07-26T18:52:21Z

src/styles/_markdown-theme.scss

+    }
+
+    &:hover {
+      .material-icons {


Looks like this is meant to target the header link but is more broad than that. Also why does it need to be absolute?

See above on re: absolute.

jelbourn · 2017-07-26T18:53:55Z

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

+  styleUrls: ['./table-of-contents.scss'],
+  template: `
+    <div *ngIf="links?.length" class="toc-container">
+      <h2>Contents</h2>


I believe <h2> isn't appropriate here; h1 - h6 are meant for content section headings, not really navigation

amcdnl · 2017-07-29T15:16:18Z

@jelbourn - ready for another review.

jelbourn · 2017-07-31T20:01:53Z

src/app/shared/doc-viewer/header-link.ts

+      class="docs-markdown-a docs-header-link"
+      aria-label="Link to this heading"
+      [href]="url">
+      <i class="material-icons">link</i>


This <i> should be aria-hidden (or use md-icon)

jelbourn · 2017-07-31T20:03:57Z

src/app/shared/doc-viewer/header-link.ts

+@Component({
+  selector: 'header-link',
+  template: `
+    <a


I think this anchor would also benefit from an aria-describedby pointing to the header

jelbourn · 2017-07-31T20:05:51Z

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

+import 'rxjs/add/observable/fromEvent';
+import 'rxjs/add/operator/debounceTime';
+
+interface Link {


Add some descriptions on this type? Not immediately obvious exactly what type, top etc. refer to or how Link in general is meant to be used

jelbourn · 2017-07-31T20:08:07Z

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

+
+  createLinks(): void {
+    // content is loading async, the timeout helps delay the query selector being empty
+    setTimeout(() => {


Would using cdkObserveContent / a MutationObserver be better for this?
(just to see when the content is updated and then can be unsubscribed)

jelbourn · 2017-07-31T20:10:43Z

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

+    setTimeout(() => {
+      const headers = this._document.querySelectorAll('.docs-markdown-h3,.docs-markdown-h4');
+      for (const header of headers) {
+        const { top } = header.getBoundingClientRect();


Omit spaces inside braces:

const {top} = header.getBoundingClientRect();

(here and elsewhere)

amcdnl · 2017-08-01T21:58:36Z

@jelbourn - ready for second review.

jelbourn · 2017-08-02T00:37:09Z

@amcdnl could you rebase?
(I merged your other PR for inline styles)

amcdnl · 2017-08-05T15:32:01Z

Done :)

jelbourn · 2017-08-07T17:06:47Z

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

+          {{link.name}}
+        </a>
+      </nav>
+    </div>


Move this template into its own file?

jelbourn · 2017-08-07T17:09:00Z

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

+  private _fragmentObserver: MutationObserver;
+  private _headerObserver: MutationObserver;
+
+  private get scrollOffset(): number {


This would be a better function than a getter.
(getters generate a lot of JS code vs just a function)

jelbourn · 2017-08-07T17:09:35Z

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

+  private _headerObserver: MutationObserver;
+
+  private get scrollOffset(): number {
+    const {top} = this._element.nativeElement.getBoundingClientRect();


Add comment explaining what this is computing ("scrollOffset" by itself isn't specific enough)

jelbourn · 2017-08-07T17:10:12Z

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

+
+  ngOnInit(): void {
+    this._scrollContainer = this.container ?
+      this._document.querySelectorAll(this.container)[0] : window;


Why window instead of document.body or documentElement?

the body element doesn't have a height set

jelbourn · 2017-08-07T17:11:44Z

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

+  private _scrollSubscription: Subscription;
+  private _routeSubscription: Subscription;
+  private _fragmentObserver: MutationObserver;
+  private _headerObserver: MutationObserver;


Add descriptions for these two observers?

jelbourn · 2017-08-07T17:12:34Z

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

+        }
+      });
+
+      this._fragmentObserver.observe(document.body, {


Why attach this directly to the body instead of the document viewer host element?

This is attached to the body because it could be the document viewer or the guide viewer. In an effort, to keep it generic i attached it to the body. I could add an input if you like to pick a specific element to listen on. Let me know either way.

Both the component pages (component-overview and component-api) and the guide-viewer use doc-viewer in their template. Thinking about it more, though, I realize a much more straightforward approach would be to have the doc-viewer fire an event after appending the content, which the table-of-contents could use to know when to go, e.g.

<doc-viewer documentUrl="/assets/documents/overview/{{componentViewer.componentDocItem.id}}.html" class="docs-component-view-text-content docs-component-overview" (contentLoaded)="toc.doThatThing()"> </doc-viewer> <table-of-contents #toc container=".mat-sidenav-content"> </table-of-contents>

I must have had MutationObserver on my mind the other day

Lets talk through this, I'm curious to your thoughts on integrating the route change and the observer together.

jelbourn · 2017-08-07T17:14:10Z

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

+
+    // observe body content changes until the headers
+    // are painted. this is required because the page can load async
+    this._headerObserver = new MutationObserver(() => {


I don't quite follow why you need two observers set up at different times

Unfortunately the router events do not give me enough information just to attach one to the route event.

For example, I listen to this._route.fragment which returns me the route fragment a user clicked. From that fragment I then ensure its loaded and then scroll into view.

On the this._router.events, I'm listening for when the user navigates to a totally different page. For example, navigating from accordion to autocomplete. In this scenario, the page doesn't actually change since its the same route it just updates the content. In this scenario, I want to update the links only.

If the router events gave me more information, I could easily consolidate these but at this time I need 2 different subscriptions and observers to make the appropriate action.

jelbourn

Just a few last things I noticed

jelbourn · 2017-08-14T23:13:16Z

package.json

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


This needs to stay --aot

I didn't mean to commit this but AOT causes lots of issues when developing and is slow.

jelbourn · 2017-08-15T00:19:03Z

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

+  constructor(private _router: Router,
+              private _route: ActivatedRoute,
+              private _element: ElementRef,
+              @Inject(DOCUMENT) private _document: any) {


Any reason the type here can't be Document?

jelbourn · 2017-08-15T00:20:34Z

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

+      color: mat-color($foreground, secondary-text);
+
+      &:hover,
+      &.active {


Need to prefix these classes with docs-

jelbourn · 2017-08-15T00:21:02Z

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

+  font-weight: bold;
+}
+
+a {


Change you make this style a css class instead of styling a directly?

amcdnl · 2017-08-15T23:11:57Z

@jelbourn - good to go.

jelbourn

LGTM

feat(toc): add table of contents to overview and guides

d301182

amcdnl requested a review from jelbourn July 26, 2017 01:59

googlebot added the cla: yes label Jul 26, 2017

This was referenced Jul 26, 2017

chore(docs): correcting headers and adding header linking angular/components#6040

Merged

Add table-of-contents for jump navigation on docs #167

Closed

amcdnl added 2 commits July 25, 2017 21:26

Merge branch 'master' into toc

0d5d263

jelbourn reviewed Jul 26, 2017

View reviewed changes

feat(toc): address feedback angular#167

4e68acc

jelbourn reviewed Jul 31, 2017

View reviewed changes

chore(*): address feedback items

a33fd9d

Merge branch 'master' into toc

db25f6f

jelbourn reviewed Aug 7, 2017

View reviewed changes

amcdnl added 3 commits August 7, 2017 17:34

feat(toc): address PR feedback

32b431b

chore(*): remove mutation obvs

3368cdf

chore(*): fix lint

fef4b11

jelbourn reviewed Aug 15, 2017

View reviewed changes

feat(toc): nit

5c01747

jelbourn approved these changes Aug 16, 2017

View reviewed changes

jelbourn merged commit 7d0297b into angular:master Aug 16, 2017

amcdnl deleted the toc branch August 16, 2017 21:44

feat(toc): add table of contents to overview and guides #230

feat(toc): add table of contents to overview and guides #230

Uh oh!

Conversation

amcdnl commented Jul 26, 2017

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcdnl commented Jul 29, 2017

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 Aug 1, 2017

Uh oh!

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

Uh oh!

Uh oh!

amcdnl commented Aug 5, 2017

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!

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!

jelbourn commented Aug 2, 2017 •

edited

Loading