aio: add h1 title to floating table of contents by petebacondarwin · Pull Request #16959 · angular/angular

petebacondarwin · 2017-05-23T12:50:44Z

petebacondarwin · 2017-05-23T12:53:42Z

petebacondarwin · 2017-05-24T08:01:21Z

I rebased, fixed the conflict, and broke up the big commit into smaller ones to make the refactoring more easy to follow.

mary-poppins · 2017-05-24T08:10:49Z

The angular.io preview for a4fecef8b11eed8fac517db6217e35ff98459042 is available here.

mary-poppins · 2017-05-24T08:19:00Z

The angular.io preview for c496a150dd97f445293115489c98ead3ab94b2df is available here.

gkalpak

A few nit-picks. Nothing blocking.
Loved the refactorings ❤️

gkalpak · 2017-05-24T14:12:56Z

gkalpak · 2017-05-24T14:24:59Z

No whitespace after |? 😞

gkalpak · 2017-05-24T14:25:42Z

OOC, why != vs !==?

gkalpak · 2017-05-24T14:27:20Z

Ternary operator hell 😁

Nah! It's a pyramid of beauty!

Just to be clear: I like it 😃

gkalpak · 2017-05-24T14:39:20Z

I wonder if we need the element specifiers at all. Why not just .toc-heading, .toc-list .h1 { ... }?

That is probably correct. I was just sort of following the convention there already.

But I think that Stef will change some of this anyway

gkalpak · 2017-05-24T14:44:23Z

Not directly related to this PR, but I noticed this value is invalid and causes the style to be ignored. It should be 0.3s (the s is currently missing).

I'll sneak that in.

gkalpak · 2017-05-24T14:45:04Z

24px feels too much. margin: 10px 0 looks better imo 😃

I think this is due to change in Stef's PR.

gkalpak · 2017-05-24T14:49:26Z

Nit: this.tocList.filter(item => item.level !== 'h1').length is slightly more readable 😳

but slower and uses more memory :-P

I know, I know 😞 But Igor said to opt for readability over performance 😛

gkalpak · 2017-05-24T15:00:00Z

It might make sense to avoid this ngIf, by separating the h1 element from the rest (in the component).

I thought about that but it just moves the complexity somewhere else.
Not displaying the h1 IMO is a visual choice, hence the responsibility of the template.

The decision about dislaying it or not will still be in the template. But the template won't have to "extract" it from the tocList (which sounds rather "business-logic-y").
_{I don't feel at all strognly about it, though.}

I think that then you would need complexity in both the component class (to extract the h1) and then in the template (where it would need to duplicate the li element for the h1).

¯\_(ツ)_/¯

Actually also it become even more complex when you then try to get the activeIndex to line up. This way, it just works :-)

gkalpak · 2017-05-24T15:02:38Z

No test for H3 😟

petebacondarwin · 2017-05-24T16:15:59Z

@gkalpak Pushed a bunch of fixes from your feedback, some of which will be squashed. PTAL

mary-poppins · 2017-05-24T16:25:14Z

The angular.io preview for 4bb3bc7a39b01e5365ffc18b5db862db184cdabb is available here.

gkalpak

Still LGTM

gkalpak · 2017-05-24T16:20:25Z

There's one more here 😛

gkalpak · 2017-05-24T16:20:48Z

There are more missing whitespaces here.

gkalpak · 2017-05-24T16:25:23Z

return array.filter(fn).length is more readable 😛

This makes the styling less fragile to changes in the HTML

We use the `asap` scheduler because updates to `activeItemIndex` are triggered by DOM changes, which, in turn, are caused by the rendering that happened due to a ChangeDetection. Without asap, we would be updating the model while still in a ChangeDetection handler, which is disallowed by Angular.

Closes angular#16900

mary-poppins · 2017-05-24T18:01:33Z

The angular.io preview for c9f93b9 is available here.

There seems to have been a bad rebase of angular#16228 on top of angular#16959, which affected ToC styles from both PRs. This commit restores the horizontal line under `.h1` elements and the vertical blue bar on the left-hand side of the ToC (with the circle running along the bar to indicate the active section). Fixes angular#17098

There seems to have been a bad rebase of #16228 on top of #16959, which affected ToC styles from both PRs. This commit restores the horizontal line under `.h1` elements and the vertical blue bar on the left-hand side of the ToC (with the circle running along the bar to indicate the active section). Fixes #17098

* refactor(aio): use explicit CSS class for TOC container This makes the styling less fragile to changes in the HTML * fix(aio): schedule TocComponent.activeIndex updates via AsapScheduler We use the `asap` scheduler because updates to `activeItemIndex` are triggered by DOM changes, which, in turn, are caused by the rendering that happened due to a ChangeDetection. Without asap, we would be updating the model while still in a ChangeDetection handler, which is disallowed by Angular. * refactor(aio): do not instantiate floating ToC if not displayed * feat(aio): display the h1 at the top of the floating TOC Closes angular#16900 * refactor(aio): combine the TOC booleans flags into a "type" state * refactor(aio): remove unnecessary `hostElement` property * fix(aio): ensure that transition works on TOC * fix(aio): use strict equality in ToC template

There seems to have been a bad rebase of angular#16228 on top of angular#16959, which affected ToC styles from both PRs. This commit restores the horizontal line under `.h1` elements and the vertical blue bar on the left-hand side of the ToC (with the circle running along the bar to indicate the active section). Fixes angular#17098

* refactor(aio): use explicit CSS class for TOC container This makes the styling less fragile to changes in the HTML * fix(aio): schedule TocComponent.activeIndex updates via AsapScheduler We use the `asap` scheduler because updates to `activeItemIndex` are triggered by DOM changes, which, in turn, are caused by the rendering that happened due to a ChangeDetection. Without asap, we would be updating the model while still in a ChangeDetection handler, which is disallowed by Angular. * refactor(aio): do not instantiate floating ToC if not displayed * feat(aio): display the h1 at the top of the floating TOC Closes angular#16900 * refactor(aio): combine the TOC booleans flags into a "type" state * refactor(aio): remove unnecessary `hostElement` property * fix(aio): ensure that transition works on TOC * fix(aio): use strict equality in ToC template

There seems to have been a bad rebase of angular#16228 on top of angular#16959, which affected ToC styles from both PRs. This commit restores the horizontal line under `.h1` elements and the vertical blue bar on the left-hand side of the ToC (with the circle running along the bar to indicate the active section). Fixes angular#17098

angular-automatic-lock-bot · 2019-09-11T16:20:41Z

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

petebacondarwin added comp: aio action: review The PR is still awaiting reviews from at least one requested reviewer labels May 23, 2017

googlebot added the cla: yes label May 23, 2017

petebacondarwin commented May 23, 2017

View reviewed changes

Comment thread aio/src/app/embedded/toc/toc.component.ts Outdated

petebacondarwin May 23, 2017

Copy link
Copy Markdown

Contributor Author

Choose a reason for hiding this comment

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

English !!

petebacondarwin force-pushed the aio-toc-floating-title branch from a6c01b3 to a4fecef Compare May 24, 2017 08:00

petebacondarwin force-pushed the aio-toc-floating-title branch from a4fecef to c496a15 Compare May 24, 2017 08:09

gkalpak approved these changes May 24, 2017

View reviewed changes

petebacondarwin added 8 commits May 24, 2017 18:35

refactor(aio): use explicit CSS class for TOC container

42d75bd

This makes the styling less fragile to changes in the HTML

refactor(aio): do not instantiate floating ToC if not displayed

2035c56

feat(aio): display the h1 at the top of the floating TOC

84d40a7

Closes angular#16900

refactor(aio): combine the TOC booleans flags into a "type" state

6727a3f

refactor(aio): remove unnecessary hostElement property

61125b6

fix(aio): use strict equality in ToC template

42a3bf4

fix(aio): ensure that transition works on TOC

c9f93b9

petebacondarwin force-pushed the aio-toc-floating-title branch from 4bb3bc7 to c9f93b9 Compare May 24, 2017 17:51

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 May 24, 2017

chuckjaz merged commit 966eb2f into angular:master May 24, 2017

petebacondarwin deleted the aio-toc-floating-title branch May 25, 2017 10:05

gkalpak mentioned this pull request May 30, 2017

fix(aio): fix ToC styling #17110

Merged

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

Uh oh!

Conversation

petebacondarwin commented May 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petebacondarwin commented May 24, 2017

Uh oh!

mary-poppins commented May 24, 2017

Uh oh!

mary-poppins commented May 24, 2017

Uh oh!

gkalpak 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!

gkalpak May 24, 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!

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 May 24, 2017

Uh oh!

mary-poppins commented May 24, 2017

Uh oh!

gkalpak 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!

gkalpak May 24, 2017 •

edited

Loading