Add anchor for section titles for quick sharing of sections #408

marinaaisa · 2022-08-09T19:24:01Z

Bug/issue #96943502, if applicable:

Summary

Add anchor for section titles for quick sharing of sections.
This anchor shows when user hovers the title section.

Issue: #273

Dependencies

NA

Testing

Steps:

Open any docc archive
Assert that all h2 and h3 titles in documentation pages show an anchor when hovering the title

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

Added tests
Ran npm test, and it succeeded
Updated documentation if necessary

marinaaisa · 2022-08-09T19:24:26Z

@swift-ci test

marinaaisa · 2022-08-09T19:34:34Z

@swift-ci test

marinaaisa · 2022-08-10T14:52:51Z

@swift-ci test

dobromir-hristov

This looks great, very nice! 🚀

One thing I noticed is if I tab through, it does not show the anchors, deff something we should fix.

Need to check if we should disable this for IDE environments.

Also we may check with the DocC team if they plan to provide anchors for those Topic subsections, instead of us generating them - #190

Other than that, looks great :)

src/components/ContentNode/SectionTitle.vue

src/components/DocumentationTopic/ContentTableSection.vue

marinaaisa · 2022-08-11T19:28:11Z

Thanks @dobromir-hristov !

One thing I noticed is if I tab through, it does not show the anchors, deff something we should fix.

Fixed! Good catch!

Need to check if we should disable this for IDE environments.

Why would we? it should scroll in the same way, right?

Also we may check with the DocC team if they plan to provide anchors for those Topic subsections, instead of us generating them - #190

Yeah, that would be much better than the generation I'm doing in the frontend!

Give it another review. I still need to add final tests for the handleFocusAndScroll function I added in the scrollToElement mixin.

dobromir-hristov · 2022-08-11T20:00:22Z

I don't see a point in having a clickable link in the ide, since there is no URL bar

dobromir-hristov

I think it looks good now. How does VO handle the hidden anchor item? Do we need anything special for it?

dobromir-hristov · 2022-08-12T06:51:28Z

src/components/ContentNode/SectionTitle.vue

    tag: {
      type: String,
      default: () => 'h2',
    },
  },
+  mounted() {
+    if (!this.anchor) return;
+    const element = document.getElementById(`${this.anchor}`);


This is pretty clever though I am not sure how I feel about it.

All headings would be rendered initially without an id and would then re-render to add the id, if there is no element with that anchor. This makes the component easier to user, but a bit more unpredictable.

Unpredictable? We use dynamic ids in many more components across the app... 🤔
But if you have any better approach, happy to look into it :)

Well you may be right. I would have probably just made this logic user controllable, via some boolean whether to apply an id to the element or not.

marinaaisa · 2022-08-13T00:09:10Z

I think it looks good now. How does VO handle the hidden anchor item? Do we need anything special for it?

We are hiding it from VO because I think it's not providing any value to VO users. https://vuejs.org/guide/introduction.html#what-is-vue does it in the same way

marinaaisa · 2022-08-13T00:49:10Z

I don't see a point in having a clickable link in the ide, since there is no URL bar

True, I don't render it anymore on IDEs f6bd656

marinaaisa · 2022-08-13T06:09:36Z

@swift-ci test

dobromir-hristov

Looks good. @mportiz08 can you look into the anchor logic if you are fine with it and we can merge this one I think

dobromir-hristov · 2022-08-22T13:33:19Z

src/components/ContentNode/SectionTitle.vue

    tag: {
      type: String,
      default: () => 'h2',
    },
  },
+  mounted() {
+    if (!this.anchor) return;
+    const element = document.getElementById(`${this.anchor}`);


Well you may be right. I would have probably just made this logic user controllable, via some boolean whether to apply an id to the element or not.

mportiz08

Very cool to see this in action, and I love that it just works as expected in places where no anchor data is provided.

I have a minor concern about the anchor generation, but I think the majority of my other comments are pretty small things that will be easy to fix.

src/components/ContentNode.vue

src/components/ContentNode/SectionTitle.vue

mportiz08 · 2022-08-24T22:19:04Z

src/components/ContentNode/SectionTitle.vue

+  },
+  mounted() {
+    if (!this.anchor) return;
+    const element = document.getElementById(`${this.anchor}`);


Suggested change

const element = document.getElementById(`${this.anchor}`);

const element = document.getElementById(this.anchor);

As a more general comment though, I don't totally understand the need for why this hook is needed. Why are there separate id and anchor values and how are they used differently?

There are cases where the ID is applied on a section that wraps the heading (like in TopicsSection), but in other cases, the heading has to hold the ID.

I'm not totally following how that requires the mounted hook here (probably just missing something).

I think she wanted it to be more magical. Thats one of the points I raised that I wanted your input on :D

The component tries to find an already existing element with that id in the DOM, and if it doesnt, it adds that id to itself.

I guess I just don't understand the magical part, to be more clear. Why would this logic have to happen after the initial render?

So all possible items that may have that ID are rendered? As I said I would probably control this through a prop instead.

@mportiz08 I had this logic to check if the document has already an id on it where we want to link to. This happens in components that have the id in the beginning of the section itself instead of having it in the title.
Also in this way, we avoid duplication of ids in the HTML, since ID attributes must be unique in the document.

Controlling through a prop, as @dobromir-hristov suggested, would allow us more flexibility but would open the component for possible HTML's errors because we wouldn't prevent the document from duplication of HTML ids.

I understand why we might choose to sometimes assign the ID attribute to a non-heading element instead of a heading, but I don't understand why we couldn't prevent the ID from being assigned in multiple places in the components themselves—it seems like that is controlled by our app and not outside forces. If that's the case, then we should be able to programmatically decide where the anchor uniquely goes before any mounted hook gets called after the initial render.

I think we both agree on the goal of only having unique IDs but maybe there's a little confusion about how to ensure it within the components themselves. Let me know if you want to talk about this offline—I may be missing a subtle thing here that might make this approach necessary.

@mportiz08 I set a meeting to continue discussing this. Interesting debate! Thanks :)

src/components/ContentNode/SectionTitle.vue

mportiz08 · 2022-08-24T22:26:41Z

src/components/ContentNode/SectionTitle.vue

+.header-anchor {
+  margin-left: -0.73em;
+  padding-right: 0.23em;
+  transition: opacity .25s;


You may want to have someone from the design team review this new feature and test the experience in different browsers. The scrolling animation specifically (not this one) feels a little different in Safari but that may be a limitation of the JS scroll function itself.

src/components/DocumentationTopic/ContentTable.vue

src/components/DocumentationTopic/ContentTableSection.vue

src/mixins/scrollToElement.js

mportiz08 · 2022-08-24T22:42:54Z

Just noting that we may want to close #190 when this gets merged.

dobromir-hristov · 2022-08-25T08:10:19Z

Just noting that we may want to close #190 when this gets merged.

Even though this PR adds most of what #190 does, that one was assuming the anchors come from DocC. I think if Marina does that too, we can safely drop it :)

marinaaisa · 2022-08-30T18:04:20Z

Even though this PR adds most of what #190 does, that one was assuming the anchors come from DocC. I think if Marina does that too, we can safely drop it :)

I moved most of the changes from #190 PR into this one, so we can merge it in here.

marinaaisa · 2022-08-30T18:09:17Z

@swift-ci test

src/components/ContentNode/LinkableHeading.vue

src/components/ContentNode.vue

…n anchor

hqhhuang · 2022-09-01T23:48:58Z

Hey Marina!
This is brilliant! I still have questions about the id, but I will reach out offline for that.
I tested this and noticed 3 small things:

The URL seems to be case sensitive. Is this intentional?
This issue appears in Chrome only:

Screen.Recording.2022-09-01.at.4.24.57.PM.mov

Do we want to dynamically update the URL with the anchors based on where the user is on the page as they scroll? I think the Vue Doc site does this.

marinaaisa · 2022-09-02T15:13:37Z

Hey @hqhhuang ! thanks a lot for your comments!

The URL seems to be case sensitive. Is this intentional?

URL are always case sensitive. You will see that some anchors are in lowercase and others aren't. This is because some come directly from docc (some have uppercase initials) and other we add them depending on the title (these ones are in lowercase). The anchor's names don't matter if they are in lowercase or not, as long as they point to an existing id with the same name.

This issue appears in Chrome only:
Screen.Recording.2022-09-01.at.4.24.57.PM.mov

This is because of the way we scroll to the section. We first scroll into the view where the title is, and then we scrollBy to overcome the navigation height. You can see it here: https://github.com/apple/swift-docc-render/blob/f5c1b7fd271518503e85f7a5d2dd5f993ded17be/src/mixins/scrollToElement.js#L26-L27
I'm not sure yet how to prevent that jumping at the beginning, in theory, it shouldn't be noticeable... we could look into improving it in the future.

Do we want to dynamically update the URL with the anchors based on where the user is on the page as they scroll? I think the Vue Doc site does this.

Very good catch! Maybe this could a separate feature since it uses the ids but it doesn't depend on the new LinkableHeadinds, could you fill a radar or Github issue for it? Thanks!

mportiz08 · 2022-09-02T20:37:26Z

I know you're still iterating on the visual design of the link, but I wanted to say that we might want to consider some sort of absolute positioning of it to avoid situations like the following where the inline content needs to reflow sometimes:

hqhhuang · 2022-09-06T05:41:37Z

URL capitalization:

This is because some come directly from docc (some have uppercase initials) and other we add them depending on the title (these ones are in lowercase). The anchor's names don't matter if they are in lowercase or not, as long as they point to an existing id with the same name.

What are some existing examples of ones coming directly from docc? I think if we opted for all lowercase for Docc-render generated anchors, docc should also provide all lower case anchors. Is there a particular reason some of the docc ones have upper case initials?

Scroll jump:

This is because of the way we scroll to the section. We first scroll into the view where the title is, and then we scrollBy to overcome the navigation height. You can see it here:

https://github.com/apple/swift-docc-render/blob/f5c1b7fd271518503e85f7a5d2dd5f993ded17be/src/mixins/scrollToElement.js#L26-L27

Is there a difference between how Chrome and Safari handles the scroll? If this is the issue, I would sort of expect it to happen in both places.

Updating URL based on anchors:

Could you fill a radar or Github issue for it? Thanks!

rdar://99584817 I can try to work on this one in the coming weeks if we have a thumbs up from design.

dobromir-hristov

I tested and it works OK. I think you havent missed any places where we use headings.

Just make sure you fix what Marcus pointed out, with the jumping text on hover.

src/components/ContentNode/LinkableHeading.vue

marinaaisa · 2022-09-06T17:07:43Z

I know you're still iterating on the visual design of the link, but I wanted to say that we might want to consider some sort of absolute positioning of it to avoid situations like the following where the inline content needs to reflow sometimes:

@mportiz08 oh true, fixed: 23fd3c7

but yes, the design specs aren't closed yet

marinaaisa · 2022-09-07T15:34:29Z

Thanks for the feedback, @hqhhuang !

What are some existing examples of ones coming directly from docc? I think if we opted for all lowercase for Docc-render generated anchors, docc should also provide all lower case anchors.

For example in the slothcreator.json data file, the first object of the content key is:

{
  "anchor": "Overview",
  "level": 2,
  "type": "heading",
  "text": "Overview"
}

Is there a particular reason some of the docc ones have upper case initials?

I don't know. It shouldn't matter anyways

Is there a difference between how Chrome and Safari handles the scroll? If this is the issue, I would sort of expect it to happen in both places.

It may be a difference between browsers, yes! I will investigate those differences.

rdar://99584817 I can try to work on this one in the coming weeks if we have a thumbs up from design.

Thanks!

marinaaisa · 2022-09-07T15:34:43Z

@swift-ci test

…ot dissapear when hovering on it

marinaaisa · 2022-09-08T14:06:31Z

@swift-ci test

marinaaisa · 2022-09-08T14:23:55Z

@swift-ci test

marinaaisa · 2022-09-13T15:08:33Z

@swift-ci test

mportiz08

Awesome work @marinaaisa! This is working great, and I think it's almost ready to merge.

I just noticed one minor potential issue to look at with the color of the new linkable heading element.

src/components/ContentNode/LinkableHeading.vue

src/styles/core/_vars.scss

dobromir-hristov

This looks good to me, just check the things that Marcus pointed out.

SamLanier · 2022-09-15T13:55:11Z

New design approved.

marinaaisa · 2022-09-15T13:56:48Z

@swift-ci test

marinaaisa requested review from mportiz08, dobromir-hristov, hqhhuang and tarunamasa August 9, 2022 19:24

dobromir-hristov reviewed Aug 11, 2022

View reviewed changes

src/components/ContentNode/SectionTitle.vue Outdated Show resolved Hide resolved

src/components/ContentNode/SectionTitle.vue Outdated Show resolved Hide resolved

src/components/DocumentationTopic/ContentTableSection.vue Outdated Show resolved Hide resolved

marinaaisa requested a review from dobromir-hristov August 11, 2022 19:28

dobromir-hristov reviewed Aug 12, 2022

View reviewed changes

dobromir-hristov reviewed Aug 22, 2022

View reviewed changes

mportiz08 reviewed Aug 24, 2022

View reviewed changes

mportiz08 reviewed Aug 30, 2022

View reviewed changes

src/components/ContentNode/LinkableHeading.vue Outdated Show resolved Hide resolved

src/components/ContentNode.vue Outdated Show resolved Hide resolved

marinaaisa force-pushed the r96943502/anchor-titles branch from 5c89246 to 4f2840c Compare August 31, 2022 16:03

marinaaisa added 7 commits August 31, 2022 18:07

[r96943502] chore: started adding anchor to titles

36fb1ff

[r96943502] feat: create SectionTitle component

988aea7

[r96943502] feat: add SectionTitle to more headings

a288046

[r96943502] chore: make anchor optional for those that already have a…

94b47b3

…n anchor

[r96943502] test: fix current tests

a69286b

[r96943502] test: add tests for SectionTitle component

21c50ce

[r96943502] fix: TopicsTable component

2d7bfad

mportiz08 approved these changes Sep 1, 2022

View reviewed changes

marinaaisa added 2 commits September 2, 2022 19:04

[r96943502] styles: [WIP] update UI

d6b730e

[r96943502] styles: [WIP] update UI with link icon

f4877b6

dobromir-hristov reviewed Sep 6, 2022

View reviewed changes

src/components/ContentNode/LinkableHeading.vue Outdated Show resolved Hide resolved

[r96943502] fix: address feedback

23fd3c7

[r96943502] fix: adding padding right to header anchor so icon does n…

711f557

…ot dissapear when hovering on it

[r96943502] chore: add aria label for AX

e4b5f40

This was referenced Sep 8, 2022

Add anchors to ContentTableSection #190

Closed

Add on this page navigation #416

Merged

Dobromir Hristov and others added 3 commits September 12, 2022 17:10

Merge branch 'main' into r96943502/anchor-titles

ae6fd00

[r94221573] chore: address design feedback + refactor vars

2b73297

Merge branch 'main' into r96943502/anchor-titles

fa59672

mportiz08 reviewed Sep 13, 2022

View reviewed changes

src/components/ContentNode/LinkableHeading.vue Outdated Show resolved Hide resolved

src/styles/core/_vars.scss Outdated Show resolved Hide resolved

dobromir-hristov reviewed Sep 14, 2022

View reviewed changes

marinaaisa added 2 commits September 15, 2022 15:54

[r94150776] fix: address feedback

acd16a5

Merge branch 'main' into r96943502/anchor-titles

8c87dea

marinaaisa merged commit 0e106af into swiftlang:main Sep 15, 2022

marinaaisa deleted the r96943502/anchor-titles branch September 15, 2022 13:59

	const element = document.getElementById(`${this.anchor}`);
	const element = document.getElementById(this.anchor);

Add anchor for section titles for quick sharing of sections #408

Add anchor for section titles for quick sharing of sections #408

Uh oh!

Conversation

marinaaisa commented Aug 9, 2022

Summary

Dependencies

Testing

Checklist

Uh oh!

marinaaisa commented Aug 9, 2022

Uh oh!

marinaaisa commented Aug 9, 2022

Uh oh!

marinaaisa commented Aug 10, 2022

Uh oh!

dobromir-hristov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marinaaisa commented Aug 11, 2022

Uh oh!

dobromir-hristov commented Aug 11, 2022

Uh oh!

dobromir-hristov 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!

marinaaisa commented Aug 13, 2022

Uh oh!

marinaaisa commented Aug 13, 2022

Uh oh!

marinaaisa commented Aug 13, 2022

Uh oh!

dobromir-hristov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mportiz08 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mportiz08 commented Aug 24, 2022

Uh oh!

dobromir-hristov commented Aug 25, 2022

hqhhuang commented Sep 6, 2022 •

edited

Loading