Add on this page navigation #416

dobromir-hristov · 2022-08-22T11:18:55Z

Bug/issue #, if applicable: 97715869

Summary

Adds a new right sidebar component to the Documentation pages, that displays the current page headings and sections, and tracks the user as he scrolls across them.

The sidebar is only visible above 1500px if sidebar is hidden, or when the content width is above 1270px. This is done intentionally, to keep the content space untouched, so we utilise only the empty space on larger screens.

Note
In cases where a section is very small, aka there is not much content under the heading, upon navigating to that section, the section after it may get highlighted instead.
This happens because the intersection point is around 30% from the top of the screen, hence if a section is small, then the next section after it would probably reach the intersection point and get highlighted.

Dependencies

NA

Testing

Steps:

Assert that any documentation page gets the navigator rendered.
Assert that clicking on anchors navigates to that page section, keeping query parameters.

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

…ilding URLs

…isPage

franklinsch · 2022-08-22T14:12:27Z

When you click on a link in the component and it's not near the top of the page because there isn't enough content below it, the On This Page component highlights the wrong item:

Screen.Recording.2022-08-22.at.15.11.31.mov

Is this something that can be resolved?

dobromir-hristov · 2022-08-22T14:15:25Z

The intersection point is 30% of the screen. Unless I move that up, small sections would have that weird behaviour. I already pointed that out in the PR description as a note.

…wports

src/components/OnThisPageNav.vue

hqhhuang · 2022-08-25T23:51:50Z

Dobri this looks awesome!!
I just have one comment in terms of the aesthetic:
On pages without the darkened hero section, the new "on this page" section looks like it's just "floating" there. In fact, before I navigated to a page with that header, I was gonna ask why we left such a huge gap between the top of the page and the start of the nav. Do you think we could extend the gray border line more to the right and add a header to the nav?

mportiz08 · 2022-09-08T21:47:19Z

src/components/DocumentationTopic/OnThisPageSection.vue

+    level: {
+      type: Number,
+      default: 2,
+    },
  },
  created() {


I think this whole component may be obsoleted by #408 once it gets merged. Most likely we will need to rethink how this store logic should work once that happens (possibly with this PR assuming the other one is merged first).

I'm guessing that the new LinkableHeading component from that PR might be a candidate for where it would make sense to keep track of the "on this page" sections. We might also want to re-think the general logic since this created hook works great now as it did for the previous version of this component but will likely not work in a future where content is "live rendered" or "hot reloaded".

Right, I think using the LinkableHeading could be a fine candidate to register the sections in the beginning.

In the future if we have live updating of the content, this may cause issues as things will probably get out of order with, duplication and so on.

Since #408 has now been merged, this PR will need to be rebased and modified to take those changes into account.

@mportiz08 I did a merge and all looks good, nothing broke. Would you like to move all OnThisPage registration to those headings and remove this component entirely?

I think that makes sense, otherwise we kind of have some components like this which are just leftover from the old implementation and it's more confusing to follow how things work since the markup and registration is now separate. We could potentially do that later though if removing it now is too risky of a change. What do you think?

Done, 63aa095

src/components/OnThisPageNav.vue

tests/unit/components/DocumentationTopic/OnThisPageStickyContainer.spec.js

tests/unit/views/DocumentationTopic.spec.js

src/styles/core/typography/_font-styles.scss

src/components/DocumentationTopic.vue

src/components/OnThisPageNav.vue

mportiz08 · 2022-09-08T22:20:17Z

src/views/DocumentationTopic.vue

@@ -289,6 +290,9 @@ export default {
        combineVersions(topicDataDefault.schemaVersion), MIN_RENDER_JSON_VERSION_WITH_INDEX,
      ) >= 0
    ),
+    enableOnThisPageNav: ({ isTargetIDE, store }) => (
+      !isTargetIDE && store.state.onThisPageSections.length > 2


I think we might want to consider > 1 instead of > 2. Browsing the documentation for DocC, I was confused at first when the nav didn't show up, because there were only two sections (Overview and Topics), but it seemed like it would have been useful there.

CC @SamLanier

There are two debatable options here:

Always show the OTP (so it doesn't look broken when not present; assume it is always useful info)

Remove the OTP when there are >2 items; assume those items are not helpful and it is repetitive info

I can see arguments for both. @mportiz08 and @ethan-kusters I think you two have differing opinions.

My main objection here is that it's easy to end up with 2 autogenerated sections (via the declaration and topics section for example) on a page that has little to no content: https://apple.github.io/swift-argument-parser/documentation/argumentparser/argumentvisibility.

We could resolve that by looking at page height or but ignoring autogenerated content but those are both problematic in their own way.

Essentially I'd like to avoid including an "On this Page" for a page that doesn't use the scroll bar to make all the headings visible. If I can already see what's on this page by looking at the page (without scrolling), the extra navigation element is (to me) a little confusing.

🤔 we could easily check whether there is a scrollbar or not...

It still provides a highlight of the subsections that doesn't require the user manually reading and parsing through what they are—at a glance, they can see what's on the page without the actual paragraph text getting in the way. I wouldn't necessarily say it's a huge benefit or anything, but I don't think it's very distracting on short pages, especially since you have to already have a very wide screen just to see the affordance. For me personally, it would be much more confusing when it sometimes disappears without an obvious reason why. That's just my personal opinion though.

And the scenario I originally pointed out could happen even on a page with lots of non-auto-generated content (like the DocC example). For example, if you have a large overview and a topics section, like this one: https://www.swift.org/documentation/docc/

src/components/DocumentationTopic/OnThisPageStickyContainer.vue

Co-authored-by: Marcus Ortiz <[email protected]>

…s://github.com/dobromir-hristov/swift-docc-render into dhristov/r97715869-add-on-this-page-navigation

# Conflicts: # src/components/DocumentationTopic.vue

dobromir-hristov · 2022-09-16T12:04:26Z

src/components/DocumentationTopic/PrimaryContent/GenericContent.vue

@@ -31,19 +31,20 @@ export default {
  methods: {
    ...ContentNode.methods,
    addOnThisPageSections() {


I assume this would also go away if we go with the Heading registration 🤔 @mportiz08

It could, yeah—it would probably be better/simpler than this method. It will have the same issues with the live-editing scenario though I assume (as this one does).

Done - 63aa095

marinaaisa

Tested and looks good! Left minor comments

src/components/DocumentationTopic/OnThisPageStickyContainer.vue

src/components/OnThisPageNav.vue

src/stores/OnThisPageSectionsStoreBase.js

Co-authored-by: Marina Aísa <[email protected]>

…s://github.com/dobromir-hristov/swift-docc-render into dhristov/r97715869-add-on-this-page-navigation

…omponent

dobromir-hristov · 2022-09-19T11:55:14Z

@swift-ci test

dobromir-hristov · 2022-09-19T11:57:01Z

I have updated the code to register sections from within the actual LinkableHeadings.

This works great as long as the entire page gets re-rendered and not parts of it, which is the same issue we had before.

In the future, it might make more sense to refactor this behaviour but for now its fine.

marinaaisa · 2022-09-19T14:43:36Z

I have updated the code to register sections from within the actual LinkableHeadings.

Nice! Great cleaning!

I believe we are safe to delete this property and the store here, right? https://github.com/apple/swift-docc-render/blob/63aa095c600efef747b029c6a5759214f1fe22df/src/components/DocumentationTopic.vue#L317-L319

Another thing, did you discuss with @hqhhuang her comment about spacing in pages without hero? I would like to know what was the conclusion. I couldn't find your reply, sorry if it's somewhere there. This is Hanqing's comment I'm referring to:

I just have one comment in terms of the aesthetic:
On pages without the darkened hero section, the new "on this page" section looks like it's just "floating" there. In fact, before I navigated to a page with that header, I was gonna ask why we left such a huge gap between the top of the page and the start of the nav. Do you think we could extend the gray border line more to the right and add a header to the nav?

SamLanier · 2022-09-19T15:07:52Z

The divider line is only for the body content. We could look into extending is to be full width (on the left and the right) so that it could possibly work better for this new feature. @hqhhuang please file separate bug, as this is not a blocker, but something we can look at separately.

mportiz08

LGTM.

My only remaining concern is about when the UI gets enabled, but that's just a personal opinion and not a blocker either way. Nice work @dobromir-hristov!

src/components/DocumentationTopic/Summary/OnThisPageNav.vue

dobromir-hristov · 2022-09-20T06:48:01Z

@swift-ci test

dobromir-hristov · 2022-09-20T07:02:11Z

@swift-ci test

Dobromir Hristov added 10 commits August 21, 2022 12:41

feat: add OnThisPageTracker component

60eeb6c

feat: add a sticky right sidenav to docs

fe4200f

refactor: add styling to the OnThisPageTracker and fix issues with bu…

80f64c9

…ilding URLs

refactor: make sure sections without a title are not tracked for OnTh…

8945cdc

…isPage

refactor: add a smart OnThisPageStickyContainer.vue component

50ac4cc

fix: spacing issue

058aa23

chore: add tests and cleanup old code

30b3226

fix: optionally enable OnThisPageNav

4917162

fix: import path

d0e300a

fix: extra spacing

2909e7c

dobromir-hristov requested review from mportiz08, marinaaisa and hqhhuang August 22, 2022 11:18

Dobromir Hristov added 3 commits August 24, 2022 11:19

refactor: make sure OnThisPageStickyContainer is visible for more vie…

d6dc8a1

…wports

refactor: remove extra nested logic as it will not be used

e79c79e

refactor: use pre-defined font-weights

04f58b0

dobromir-hristov mentioned this pull request Aug 25, 2022

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

Merged

3 tasks

hqhhuang reviewed Aug 25, 2022

View reviewed changes

src/components/OnThisPageNav.vue Show resolved Hide resolved

Dobromir Hristov added 2 commits August 26, 2022 10:59

fix: prevent jump on scroll, when at the bottom of the page

78f05b9

fix: remove bolding

24173b5

mportiz08 reviewed Sep 8, 2022

View reviewed changes

dobromir-hristov and others added 6 commits September 9, 2022 11:07

Apply suggestions from code review

8074389

Co-authored-by: Marcus Ortiz <[email protected]>

refactor: set the section's anchor as current

caebb3c

Merge branch 'dhristov/r97715869-add-on-this-page-navigation' of http…

3dc8979

…s://github.com/dobromir-hristov/swift-docc-render into dhristov/r97715869-add-on-this-page-navigation

refactor: remove on-this-page font style for body-reduced-tight

7a05c87

refactor: OnThisPageNav scoped slot

79555cb

Merge branch 'main' into dhristov/r97715869-add-on-this-page-navigation

b06756a

# Conflicts: # src/components/DocumentationTopic.vue

refactor: use mixin focusAndScroll method

3e15a92

dobromir-hristov commented Sep 16, 2022

View reviewed changes

marinaaisa approved these changes Sep 16, 2022

View reviewed changes

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

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

src/stores/OnThisPageSectionsStoreBase.js Outdated Show resolved Hide resolved

dobromir-hristov and others added 6 commits September 19, 2022 13:46

Apply suggestions from code review

5760958

Co-authored-by: Marina Aísa <[email protected]>

refactor: copy date

1d2929b

Merge branch 'dhristov/r97715869-add-on-this-page-navigation' of http…

20fd172

…s://github.com/dobromir-hristov/swift-docc-render into dhristov/r97715869-add-on-this-page-navigation

refactor: exit early the onScroll function

177efa5

refactor: register OnThisPage Sections inside the LinkableHeading c…

63aa095

…omponent

Merge branch 'main' into dhristov/r97715869-add-on-this-page-navigation

415dac2

mportiz08 approved these changes Sep 19, 2022

View reviewed changes

src/components/DocumentationTopic/Summary/OnThisPageNav.vue Show resolved Hide resolved

refactor: cleanup unused state

0b576c7

chore: remove unused components

85effce

dobromir-hristov force-pushed the dhristov/r97715869-add-on-this-page-navigation branch from 6a7c91f to 85effce Compare September 20, 2022 07:01

Merge branch 'main' into dhristov/r97715869-add-on-this-page-navigation

05fc0f9

dobromir-hristov merged commit b6da958 into swiftlang:main Sep 20, 2022

dobromir-hristov deleted the dhristov/r97715869-add-on-this-page-navigation branch September 20, 2022 07:10

Add on this page navigation #416

Add on this page navigation #416

Uh oh!

Conversation

dobromir-hristov commented Aug 22, 2022

Summary

Dependencies

Testing

Checklist

Uh oh!

franklinsch commented Aug 22, 2022

Uh oh!

dobromir-hristov commented Aug 22, 2022

Uh oh!

Uh oh!

hqhhuang commented Aug 25, 2022

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!

Uh oh!

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!

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!

marinaaisa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dobromir-hristov commented Sep 19, 2022

Uh oh!

dobromir-hristov commented Sep 19, 2022

Uh oh!

marinaaisa commented Sep 19, 2022

Uh oh!

SamLanier commented Sep 19, 2022

Uh oh!

mportiz08 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dobromir-hristov commented Sep 20, 2022

Uh oh!

dobromir-hristov commented Sep 20, 2022