feat(aio): add embedded TOC component #16078

wardbell · 2017-04-18T08:58:50Z

WHAT IS IT?

Adds an expando embedded Table of Contents component (<aio-toc>) that works much like
this google web documentation page.

Update April 26

The latest commit is a complete rewrite based on feedback and discussion. The older commits have been squashed.

The current PR has the following characteristics:

The TOC is automatically generated and placed below the first (only) <h1> which serves as the document title.
The author cannot supply own TOC contents.
The author can opt out by adding the no-toc class to the <h1>. That's important for many pages, especially marketing pages.
The service creates an array of TOC info rather than HTML
The TocComponent uses standard templating to display the TOC info
A TOC heading can include HTML formatting.
Sets the browser tab title as a by-product of finding the <h1>
Tests for all features

This PR does not yet display the TOC in a third panel on the right when the viewport is wide.

Original Commits

Second commit shows the <aio-toc> component at work in the "Angular Modules" guide, ngModules.md.

Generating the TOC

Sometimes the author wants to control the TOC by hand which is why this component can and will prefer to use projected content.

THEN ... in a subsequent PR, we are going to tackle how to generate the TOC when the author simply drops <aio-toc></aio-toc> on the page where the TOC should go.

A TOC is not required. Some docs are too short to benefit from a TOC. Nor is its position on the page easily calculated. Therefore, the author must opt-in to the TOC by placing it on the page, with or without TOC-content.

I discussed separately with @petebacondarwin whether dgeni or the app should generate the TOC-content when the author omits it. While dgeni can do the job ahead of time, it can't account for the potential expansion of runtime, embedded components which might add headings to the page.

Moreover it is possible that the TOC component will move around. It's in the doc body in normal width but on wide screens it could appear in a 3rd panel to the right as it does on the Google Web docs.

Accordingly, we agreed that the doc viewer will calculate the TOC (if needed) with the aid of a service, after embedded components have rendered.

That is Part 2 of this PR (in the 3rd commit). The "Animations" page demonstrates TOC generation.

Remember to run yarn docs to see it in action.

Todo

Add tests
Show in separate pane on the right in wide mode (DO IN SEPARATE PR)

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

mary-poppins · 2017-04-18T16:32:55Z

The angular.io preview for 415f3fc76ea21580942c1ee0f543d4e02a5a531c is available here.

gkalpak · 2017-04-19T12:57:29Z

It can use some styling love, but generally LGTM. I think it is quite useful (especially for longer pages).
👍 for have it as a fixed/floating pane on wider screens.
👍 for auto-generating.

petebacondarwin · 2017-04-19T13:17:03Z

aio/src/app/embedded/code/code-example.component.ts

@@ -19,7 +19,7 @@ import { Component, ElementRef, OnInit } from '@angular/core';
    <aio-code [ngClass]="{'headed-code':title, 'simple-code':!title}" [code]="code" [language]="language" [linenums]="linenums"></aio-code>
  `
 })
-export class CodeExampleComponent implements OnInit { // implements AfterViewInit {
+export class CodeExampleComponent implements OnInit {


petebacondarwin

Do you specifically want to create these TOCs manually? Would it not be cleaner (and less maintenance for authors) if we parsed the document after it is rendered and genearate the TOC from all the H2s, etc.

IgorMinar · 2017-04-19T14:59:29Z

I agree with Pete, TOC should be auto generated by dgeni.

We should just put a placeholder for dgeni to know where to drop the TOC and then dgeni can do the rest (especially once the header anchor PR is merged, this will be trivial and less error prone)

IgorMinar

I agree with Pete, TOC should be auto generated by dgeni.

We should just put a placeholder for dgeni to know where to drop the TOC and then dgeni can do the rest (especially once the header anchor PR is merged, this will be trivial and less error prone)

mary-poppins · 2017-04-19T22:51:45Z

The angular.io preview for 05819207b05e66e181a7b81e80b944443c8fd3df is available here.

wardbell · 2017-04-19T22:53:11Z

Regarding TOC generation. I thought about that when I started this journey. I should have explained in the opening comment. I have updated it with our ( @petebacondarwin and my) plan.

wardbell · 2017-04-20T10:51:05Z

Ready to explore. No tests yet so still WIP.

yarn docs
Navigate to "Angular Modules" guide to see <aio-toc>...</aio-toc> when author specifies the TOC with a <ul> list.
Navigate "Animations" guide to see <aio-toc></aio-toc> (no list), which leads to auto-generation of the TOC from H2 and H3.

The TOC generator will use heading ids if present. If missing, it creates them.

IgorMinar · 2017-04-20T11:11:33Z

build failed, so there is no preview.

IgorMinar

Can you please explain why we should implement this functionality in dgeni? it already does all the anchor handling and link checking, so it's only natural that it will also do TOC generation.

IgorMinar · 2017-04-20T10:34:04Z

aio/src/app/shared/toc.service.ts

+
+  setDoc(doc: DocumentContents, docElement: ElementRef) {
+    this.doc = doc;
+    this.docElement = docElement.nativeElement;


maybe rename the docElement to something else? it's confusing that you have two variables of the same name but different types.

IgorMinar · 2017-04-20T10:36:40Z

aio/src/app/shared/toc.service.ts

+  setDoc(doc: DocumentContents, docElement: ElementRef) {
+    this.doc = doc;
+    this.docElement = docElement.nativeElement;
+    this.url = doc.url || '';


url -> docUrl?

IgorMinar · 2017-04-20T10:41:15Z

aio/src/app/embedded/toc/toc.component.ts

+  hasContent = true; // assume it does until we know otherwise
+  hasSecondary = true;
+  isClosed = true;
+  isEmbedded = true;


what's the difference between embedded and isEmbedded?

IgorMinar · 2017-04-20T10:44:29Z

aio/src/app/shared/toc.service.ts

+      // is always authored by the documentation team and is considered to be safe
+      const li = this.document.createElement('li');
+      const html =  `<a href="${this.url}#${id}" title="${heading.innerText}">${heading.innerHTML}</a>`;
+      li.innerHTML = html;


why do you use innerHTML here? It's unnecessary dangerous and error prone. In the other places we didn't have another option, so we had to use innerHTML, but here we can use regular Angular templating.

IgorMinar · 2017-04-20T10:47:02Z

aio/src/app/shared/toc.service.ts

+      }
+
+      // security: the document which provides this heading content
+      // is always authored by the documentation team and is considered to be safe


this is not true. it can't be considered safe. you are mixing various inputs into a string that will result in html and later dom. it's hard to confidently say that all combinations of possible data will be safe when concatenated below. It will take just one heading to contain double quote and your string concat will break.

IgorMinar · 2017-04-20T10:59:23Z

aio/src/app/shared/toc.service.ts

+    return tocElements;
+  }
+
+  private getId(h: HTMLHeadingElement, idMap: Map<string, number>) {


remove per comment above

IgorMinar · 2017-04-20T11:07:47Z

aio/src/app/embedded/toc/toc.component.ts

+  }
+
+  private setTocList(el: Element) {
+    setTimeout(() => {


this is a big red flag. Whatever you are doing here is likely either bad design or a bug in Angular. Since I see that you are recreating your own templating engine above, I suggest that you stop doing that and use Angular templating instead of custom dom manipulation.

IgorMinar · 2017-04-20T11:08:28Z

aio/src/app/embedded/toc/toc.component.ts

+      // Worse to use `AfterViewInit` because several bound properties can change
+      // triggering the dreaded "property value changed after checked" error.
+
+      // Security: the aioTocContent comes from the pre-rendered DOM and is considered to be secure


not true. by now you have no idea what is being appended because you processed the DOM in many ways and you don't know the origin and interaction of various interpolated parts.

IgorMinar · 2017-04-20T11:08:52Z

aio/src/app/shared/toc.service.spec.ts

@@ -0,0 +1,23 @@
+// TODO: WRITE TESTS


IgorMinar · 2017-04-20T11:14:46Z

aio/src/app/embedded/toc/toc.component.html

@@ -0,0 +1,20 @@
+<div *ngIf="hasContent" [class.embedded]="isEmbedded" [class.closed]="isClosed">
+  <div *ngIf="!hasSecondary"class="toc-heading">Contents</div>
+  <div *ngIf="hasSecondary" class="toc-heading secondary"


does this really need to be this complicated? it's just a TOC :-)

why do we need the ability to close the TOC? if we really do need that functionality, shouldn't we use the material zippy instead?

IgorMinar · 2017-04-20T11:27:05Z

aio/src/app/embedded/toc/toc.component.ts

+    const content = this.elementRef.nativeElement.aioTocContent.trim();
+    this.hasContent = !!content;
+
+    if (this.hasContent) {


do we really want to allow manually created TOC? why have two ways of doing this? Let's just have one. Can we remove this option?

petebacondarwin · 2017-04-25T11:29:08Z

I spoke with @IgorMinar.
We want to generate the TOC data as part of the doc-gen, and add this to the DocumentContents data that is sent down the wire to the DocViewerComponent. This component will then render this TOC in a separate element to the document contents itself that should be attached the top right of the page.

mary-poppins · 2017-04-27T06:27:55Z

The angular.io preview for bdc0ccac4dff1129ba3e1bcad15e26a33c2f9fc1 is available here.

petebacondarwin · 2017-04-27T14:08:35Z

aio/src/app/embedded/toc/toc.component.spec.ts

@@ -0,0 +1,215 @@
+// TODO: WRITE TESTS


perhaps you can remove this now?

petebacondarwin · 2017-04-27T14:11:05Z

aio/src/app/layout/doc-viewer/doc-viewer.component.ts

+    let title = '';
+    const titleEl = this.hostElement.querySelector('h1');
+    // Only create TOC for docs with an <h1> title
+    // If you don't want a TOC, don't have an <h1>


The home page will have an h1 right? But I guess it shouldn't have a TOC.

It will be marked with no-toc class and thus skipped

Maybe this is worth putting into the comment for posterity.

petebacondarwin

At a very high level this looks awesome! I'll pick through it tonight.

Addressed all points. We all agreed to do the TOC in the client.

mary-poppins · 2017-04-27T17:24:53Z

The angular.io preview for 40e6e2c281194dcb6bf7bd4c9899b00860b689ed is available here.

mary-poppins · 2017-04-27T17:27:22Z

The angular.io preview for 395476d27975b274293719c4addbd9c358e9f81d is available here.

mary-poppins · 2017-04-27T19:14:10Z

The angular.io preview for eb28382 is available here.

petebacondarwin

This PR works for me. I couldn't see any major issues with the implementation. I guess we need to implement the floating ToC now too?

gkalpak

The file-not-found and fetching-error content need a no-toc classes.

gkalpak · 2017-04-27T20:35:08Z

aio/src/app/embedded/toc/toc.component.html

@@ -0,0 +1,24 @@
+<div *ngIf="hasToc" [class.closed]="isClosed">
+  <div *ngIf="!hasSecondary"class="toc-heading">Contents</div>


Nit: Missing space before class (unless it is intentional to align with the one below 😛)

gkalpak · 2017-04-27T20:38:15Z

aio/src/app/layout/doc-viewer/doc-viewer.component.ts

+    let title = '';
+    const titleEl = this.hostElement.querySelector('h1');
+    // Only create TOC for docs with an <h1> title
+    // If you don't want a TOC, don't have an <h1>


Maybe this is worth putting into the comment for posterity.

gkalpak · 2017-04-27T20:42:53Z

aio/src/styles/2-modules/_toc.scss

+}
+
+@media screen and (max-width: 1200px) {
+  aio-toc.embedded:not(:empty) {


This selector does not work. :empty will not match even if there is only whitespace in the element.

I copied it from the google web doc example and don't know what it does. I'm not testing it either. Removing.

petebacondarwin · 2017-04-27T21:01:34Z

@wardbell - marked for merge. Feel free to fix up @gkalpak's comments if you have time. Otherwise, please create issues to track them for fixing later.

wardbell · 2017-04-27T22:44:47Z

Implemented @gkalpak suggestions in PR #16391

… documents Related to angular#16078.

… documents Related to #16078.

… documents Related to angular#16078.

angular-automatic-lock-bot · 2019-09-11T11:20:38Z

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.}

wardbell added the comp: aio label Apr 18, 2017

wardbell self-assigned this Apr 18, 2017

googlebot added the cla: yes label Apr 18, 2017

wardbell force-pushed the aio-toc-component branch 3 times, most recently from 01af6f0 to 415f3fc Compare April 18, 2017 16:15

wardbell mentioned this pull request Apr 18, 2017

aio: add responsive TOC widget #14885

Closed

wardbell added the action: review The PR is still awaiting reviews from at least one requested reviewer label Apr 19, 2017

petebacondarwin reviewed Apr 19, 2017

View reviewed changes

IgorMinar added state: WIP and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 19, 2017

IgorMinar self-requested a review April 19, 2017 15:00

IgorMinar suggested changes Apr 19, 2017

View reviewed changes

wardbell force-pushed the aio-toc-component branch from 415f3fc to 0581920 Compare April 19, 2017 22:36

wardbell force-pushed the aio-toc-component branch 2 times, most recently from 4ec6987 to 88a996e Compare April 20, 2017 10:46

IgorMinar previously requested changes Apr 20, 2017

View reviewed changes

IgorMinar reviewed Apr 20, 2017

View reviewed changes

wardbell force-pushed the aio-toc-component branch from 88a996e to 880dc39 Compare April 26, 2017 00:48

petebacondarwin mentioned this pull request Apr 26, 2017

feat(aio): implement jumpNav #14139

Closed

wardbell force-pushed the aio-toc-component branch from 33b9108 to 5a828cc Compare April 26, 2017 11:25

wardbell removed the state: WIP label Apr 27, 2017

petebacondarwin reviewed Apr 27, 2017

View reviewed changes

wardbell force-pushed the aio-toc-component branch from bdc0cca to 40e6e2c Compare April 27, 2017 17:07

wardbell force-pushed the aio-toc-component branch from 40e6e2c to 395476d Compare April 27, 2017 17:12

feat(aio): add Table of Contents (toc) component.

eb28382

wardbell force-pushed the aio-toc-component branch from 395476d to eb28382 Compare April 27, 2017 18:57

petebacondarwin approved these changes Apr 27, 2017

View reviewed changes

gkalpak approved these changes Apr 27, 2017

View reviewed changes

petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 27, 2017

mhevery merged commit 3f46645 into angular:master Apr 27, 2017

wardbell mentioned this pull request Apr 27, 2017

refactor(aio): implement TOC gkalpak feedback #16078 #16391

Merged

3 tasks

gkalpak added a commit to gkalpak/angular that referenced this pull request Apr 28, 2017

fix(aio): do not create ToC for file-not-found and fetching-error…

149be7d

… documents Related to angular#16078.

gkalpak mentioned this pull request Apr 28, 2017

fix(aio): do not create ToC for file-not-found and fetching-error documents #16407

Merged

petebacondarwin pushed a commit that referenced this pull request Apr 28, 2017

fix(aio): do not create ToC for file-not-found and fetching-error…

c04e51c

… documents Related to #16078.

asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017

feat(aio): add Table of Contents (toc) component. (angular#16078)

193f514

asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017

fix(aio): do not create ToC for file-not-found and fetching-error…

8679e0f

… documents Related to angular#16078.

juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017

feat(aio): add Table of Contents (toc) component. (angular#16078)

9e95501

juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017

fix(aio): do not create ToC for file-not-found and fetching-error…

b4cf3c5

… documents Related to angular#16078.

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019

		@@ -0,0 +1,24 @@
		<div *ngIf="hasToc" [class.closed]="isClosed">
		<div *ngIf="!hasSecondary"class="toc-heading">Contents</div>

feat(aio): add embedded TOC component #16078

feat(aio): add embedded TOC component #16078

Uh oh!

Conversation

wardbell commented Apr 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHAT IS IT?

Update April 26

Original Commits

Uh oh!

mary-poppins commented Apr 18, 2017

Uh oh!

gkalpak commented Apr 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

IgorMinar commented Apr 19, 2017

Uh oh!

IgorMinar left a comment

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Apr 19, 2017

Uh oh!

wardbell commented Apr 19, 2017

Uh oh!

wardbell commented Apr 20, 2017

Uh oh!

IgorMinar commented Apr 20, 2017

Uh oh!

IgorMinar left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petebacondarwin commented Apr 25, 2017

Uh oh!

mary-poppins commented Apr 27, 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!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Apr 27, 2017

Uh oh!

mary-poppins commented Apr 27, 2017

wardbell commented Apr 18, 2017 •

edited

Loading

IgorMinar left a comment •

edited

Loading

gkalpak Apr 27, 2017 •

edited

Loading