fix(aio): improve heading-anchor placement by petebacondarwin · Pull Request #16336 · angular/angular

petebacondarwin · 2017-04-26T07:37:08Z

No description provided.

mary-poppins · 2017-04-26T07:53:22Z

The angular.io preview for ad70ba06950170b67f9e069dfd1e6a724ecaf1a3 is available here.

gkalpak · 2017-04-26T08:38:29Z

docs_test fails (it seems that some assertions need to be updated).

petebacondarwin · 2017-04-26T08:41:28Z

Good old tests :-)

petebacondarwin · 2017-04-26T08:45:02Z

Fixed the tests.

gkalpak · 2017-04-26T08:56:43Z

Not super-excited about having them on the right, but other than that it LGTM (as long as Travis is happy).

mary-poppins · 2017-04-26T09:00:39Z

The angular.io preview for 2eb8cf91b5489990e32bbf403c645fa35ab9e8ff is available here.

petebacondarwin · 2017-04-26T09:41:04Z

I noticed that @sjtrimble's new PR (#16327) puts a heading anchor on the far right (and doesn't make it invisible when not hovering. This may have been my accident but it looked quite nice.
So I have updated this PR to achieve the same across all headings.

(I also took this as an opportunity to fix the concern that @gkalpak had about the absolute positioning of icons by a generic CSS rule was too broad).

Let me know what you think.

mary-poppins · 2017-04-26T09:54:22Z

The angular.io preview for ad27107e356032f2474c204dcf84b974f0cdb0ad is available here.

gkalpak · 2017-04-26T10:26:49Z

I am not a huge fan of far-right, always-visible anchors (but I can live with that if we can't find a better solution).

Here's another idea:
If the cases where there is a bordered parent (that messs with the left-side anchor) are limited to a couple known ones, maybe it is worth keeping the left-side anchors (possibly keeping them visible with low opacity when not hovered to make them more discoverable) and special-cases the few situations where there are bordered parents (via CSS styles).

gkalpak

Implementation-wise LGTM (with the tests fixed) 👍

petebacondarwin · 2017-04-26T10:40:38Z

I just realised this breaks copy button positioning. At least on my phone

…

On Wed, 26 Apr 2017, 11:29 George Kalpakas, ***@***.***> wrote: ***@***.**** approved this pull request. Implementation-wise LGTM (with the tests fixed) 👍 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#16336 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAA9JwsKcoAu9nizD3B9m4ArIQpfKy52ks5rzxxygaJpZM4NIehE> .

gkalpak · 2017-04-26T11:13:04Z

This should probably be right: 0.

gkalpak · 2017-04-26T11:14:07Z

I don't see any difference with this PR (vs master). Positioning on mobile is probably already broken (see https://github.com/angular/angular/pull/16336/files#r113424809).
(Unless you mean something else.)

petebacondarwin · 2017-04-26T13:11:22Z

I fixed the mobile alignment. And I also made it invisible again.

mary-poppins · 2017-04-26T13:20:52Z

The angular.io preview for 8dbca6afbf7cfb0770d3b4491b8322bf48e669d2 is available here.

sjtrimble · 2017-04-26T16:09:28Z

@petebacondarwin It wasn't my intent to put the anchor on the right :) but I know this might fix some of the header indentation issues for now. The side effect of your updates is having the h1 before element (the line under the h1) extend a bit further out than planned. Can we make the width 10% instead?

petebacondarwin · 2017-04-26T16:23:44Z

Let's check this is what we actually want before you make any more changes

…

On Wed, 26 Apr 2017, 17:11 Stefanie Fluin, ***@***.***> wrote: @petebacondarwin <https://github.com/petebacondarwin> It wasn't my intent to put the anchor on the right :) but I know this might fix some of the header indentation issues for now. The side effect of your updates is having the h1 before element (the line under the h1) extend a bit further out than planned. I will submit an update. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16336 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAA9J-zB5Vx0wyHFi4kj7ahnC4QeMSb7ks5rz2xrgaJpZM4NIehE> .

petebacondarwin · 2017-04-26T19:34:17Z

In our meeting we agreed to move the anchor back to the left side and change the CSS so that if the heading is inside a subsection the anchor is moved further left so that from a user's perspective the anchor always appears in the left gutter of the page.

mary-poppins · 2017-04-26T21:51:46Z

The angular.io preview for dc8b9d4423cd66f4bc7b9fa4bfbb4ae79214676e is available here.

wardbell · 2017-04-26T21:53:37Z

@petebacondarwin I like the new anchor-in-the-gutter. Looking forward to seeing it work with HTML heading tags as well as MD heading tags. Thanks!

This package will allow us to do complex post-processing on the HTML that is rendered by Nunjucks.

Previously the CSS styling for material icons was too broad and affected all instances of icons. This commit constrains the position of copy button icons only to copy buttons.

The autolinking is now done on the `renderedContent` which means it also captures and autolinks headings that were generated outside of markdown.

mary-poppins · 2017-04-27T14:05:06Z

The angular.io preview for e5f23ae is available here.

gkalpak · 2017-04-27T20:57:25Z

-    /* Support for all WebKit browsers. */
-    -webkit-font-smoothing: antialiased;
-    /* Support for Safari and Chrome. */
-    text-rendering: optimizeLegibility;


Does @sjtrimble know you did this? 😁

gkalpak · 2017-04-27T21:17:20Z

+ *
+ * Each plugin is a factory function that will be called with the "rehype" engine as `this`.
+ * The factory should return a function that takes a HAST and returns a `boolean` or `undefined`.
+ * The HAST can be mutated by the this function.


the this --> the or this

The this variable

I still think you meant "The HAST can be mutated by this function(where "this function" is the _"function that takes a HAST and returns abooleanorundefined`"_, which in turn is returned by the plugin factory function) 😁

Oh yeah :-)

Fixes angular#16336 (comment)

Fixes #16336 (comment)

The autolinking is now done on the `renderedContent` which means it also captures and autolinks headings that were generated outside of markdown. PR Close angular#16336

Fixes angular#16336 (comment)

The autolinking is now done on the `renderedContent` which means it also captures and autolinks headings that were generated outside of markdown. PR Close angular#16336

Fixes angular#16336 (comment)

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

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 type: bug/fix labels Apr 26, 2017

googlebot added the cla: yes label Apr 26, 2017

petebacondarwin force-pushed the aio-anchor-link-placement branch from ad70ba0 to 2eb8cf9 Compare April 26, 2017 08:44

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

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

petebacondarwin force-pushed the aio-anchor-link-placement branch from 2eb8cf9 to ad27107 Compare April 26, 2017 09:38

gkalpak approved these changes Apr 26, 2017

View reviewed changes

gkalpak reviewed Apr 26, 2017

View reviewed changes

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

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

petebacondarwin force-pushed the aio-anchor-link-placement branch from 8dbca6a to dc8b9d4 Compare April 26, 2017 21:36

petebacondarwin changed the title ~~fix(aio): place anchor links after heading text~~ fix(aio): improve heading-anchor placement Apr 26, 2017

petebacondarwin added 4 commits April 27, 2017 14:37

build(aio): add new post-process dgeni package

719e07a

This package will allow us to do complex post-processing on the HTML that is rendered by Nunjucks.

fix(aio): ensure styling of icons is specific to usage

81c9784

Previously the CSS styling for material icons was too broad and affected all instances of icons. This commit constrains the position of copy button icons only to copy buttons.

fix(aio): improve positioning of heading-anchors

84dc8e0

build(aio): move autolink-headings to post-processing

e5f23ae

The autolinking is now done on the `renderedContent` which means it also captures and autolinks headings that were generated outside of markdown.

petebacondarwin force-pushed the aio-anchor-link-placement branch from dc8b9d4 to e5f23ae Compare April 27, 2017 13:46

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

gkalpak approved these changes Apr 27, 2017

View reviewed changes

wardbell 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 28, 2017

mhevery closed this in de25cfc Apr 28, 2017

petebacondarwin deleted the aio-anchor-link-placement branch April 28, 2017 09:01

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request May 1, 2017

style(aio): fix comment on postProcessHtml processor

da9cb90

Fixes angular#16336 (comment)

matsko pushed a commit that referenced this pull request May 1, 2017

style(aio): fix comment on postProcessHtml processor

3b80472

Fixes #16336 (comment)

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

style(aio): fix comment on postProcessHtml processor

1615ad7

Fixes angular#16336 (comment)

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

style(aio): fix comment on postProcessHtml processor

0ed207c

Fixes angular#16336 (comment)

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

Conversation

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

Uh oh!

Uh oh!

mary-poppins commented Apr 26, 2017

Uh oh!

gkalpak commented Apr 26, 2017

Uh oh!

petebacondarwin commented Apr 26, 2017

Uh oh!

petebacondarwin commented Apr 26, 2017

Uh oh!

gkalpak commented Apr 26, 2017

Uh oh!

mary-poppins commented Apr 26, 2017

Uh oh!

petebacondarwin commented Apr 26, 2017

Uh oh!

mary-poppins commented Apr 26, 2017

Uh oh!

gkalpak commented Apr 26, 2017

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

petebacondarwin commented Apr 26, 2017 via email

Uh oh!

gkalpak Apr 26, 2017

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Apr 26, 2017

Choose a reason for hiding this comment

Uh oh!

gkalpak commented Apr 26, 2017

Uh oh!

petebacondarwin commented Apr 26, 2017

Uh oh!

mary-poppins commented Apr 26, 2017

Uh oh!

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

Uh oh!

Uh oh!

petebacondarwin commented Apr 26, 2017 via email

Uh oh!

petebacondarwin commented Apr 26, 2017

Uh oh!

mary-poppins commented Apr 26, 2017

Uh oh!

wardbell commented Apr 26, 2017

Uh oh!

mary-poppins commented Apr 27, 2017

Uh oh!

gkalpak Apr 27, 2017

Choose a reason for hiding this comment

Uh oh!

gkalpak Apr 27, 2017

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Apr 27, 2017

Choose a reason for hiding this comment

Uh oh!

gkalpak Apr 28, 2017

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Apr 28, 2017

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot Bot commented Sep 11, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

petebacondarwin commented Apr 26, 2017 •

edited

Loading

sjtrimble commented Apr 26, 2017 •

edited

Loading