feat: highlight nav item in onPageNav ToC #1524

Thanks for the PR! I'm in favor of adding this feature but not sure whether it should be the default or opt-in. One argument against making it default is that this feature is not that useful on mobile and it'll be wasted bytes shipped to mobile clients. Anyway, few things that needs to be addressed before we can merge this:

This PR does more than just "highlight nav item in onPageNav ToC", there's also scrolling to a heading when we click on the TOC. I don't think we want that behavior. The scrolling for certain items can take very long (although this probably can be configured); the browser default behavior of jumping directly to the heading is fine. For e.g. go to https://deploy-preview-1524--docusaurus-preview.netlify.com/docs/en/publishing#building-static-html-pages and click on "Publishing to GitHub Enterprise". It takes around a good 3s for it to scroll down and while the animation is nice, I think that takes far too long and is at the point of annoyance.
Make the code ES5 friendly so that it works on older browsers hopefully (IE11).
Fix some styling nits in the CSS.

@wgao19 @endilie have a look too. What do you all think?

packages/docusaurus-1.x/lib/core/Head.js

packages/docusaurus-1.x/lib/static/css/main.css

packages/docusaurus-1.x/lib/static/js/scrollSpy.js

endiliey · 2019-05-29T05:05:27Z

Sorry but I’m not in favor of this PR.

First, it can be easily added by the user as a script. Katex did it (i can argue that the code is similar too) https://github.com/KaTeX/KaTeX/blob/master/website/static/js/scrollspy.js

Second, we are working on v2 and this means we have to port it back to v2. With such limited manpower (nobody is working on docusaurus full-time), its just going to delay v2 even more.

Third, new feature can come with a new bug. And eventually if bug arise, the maintainer will be the one having to fix it.

I think we need to make a clear stance that we are less likely to accept new feature as we are working on v2. Unless there is a high demand on it.

I really appreciate you taking your time to send this PR. I wish the effort was sent towards v2 instead

wgao19 · 2019-05-29T06:32:03Z

I like this addition! Feels very natural to have the highlighting follow scrolling.

I agree with @yangshun in favor of the browser's default behavior. I'd say smooth scroll on header clicks is at most an opt-in.

Checked a few non-Docusaurus doc sites on this issue:

Doc site	Highlight follows scroll	Smooth scroll to header clicks
ReactJS	No	No
VueJS	Yes	Yes
Docz	Yes	No
Google Web	Yes	No

tusharf5 · 2019-05-29T08:16:05Z

Thank you all for the great feedback. @endiliey If this PR gets merged then I can certainly work on sending a PR that replicates this behavior in v2 also.

endiliey · 2019-05-29T08:16:23Z

I'm going to let yall decide. My humble request and only complain is that we have to port this to v2 first before merging it. Not a good idea to see it in v1 but not in v2.

This was unexpected as well because there seems to be no discussion regarding this and suddenly the PR came out

endiliey · 2019-05-29T08:17:11Z

Thank you all for the great feedback. @endiliey If this PR gets merged then I can certainly work on sending a PR that replicates this behavior in v2 also.

Oh cool, that'll be awesome then. Then my only complain is pretty much gone

tusharf5 · 2019-05-29T10:40:30Z

@yangshun I've made some changes according to the feedback.

Added a smoothScroll option in siteConfig to enable smooth scroll.
Combined the two scrollSpy scripts as one in ES5.
Update the documentation

Let me know what you think of this.

yangshun

Sorry, we don't want the smooth scroll at all, only the highlighting of TOC when scrolling. Even if we want smooth scroll it should be a separate PR. This PR title doesn't accurately reflect the contents.

tusharf5 · 2019-05-29T11:00:54Z

@yangshun I've removed smooth scroll from this PR. Let me know if a separate PR for that would make sense : )

yangshun

Thanks! This looks better. Just a few more issues before we can merge it!

yangshun · 2019-05-30T05:40:43Z

packages/docusaurus-1.x/lib/static/js/scrollSpy.js

+      // Not visible.  Scroll sidebar.
+      item.scrollIntoView({block: 'center', inline: 'nearest'});
+      // eslint-disable-next-line no-multi-assign
+      document.body.scrollTop = document.documentElement.scrollTop = 0;


Can you explain what these are for?

I realized these lines were unnecessary so I've removed them.

yangshun · 2019-05-30T05:41:17Z

packages/docusaurus-1.x/lib/static/css/main.css

@@ -725,7 +725,7 @@ blockquote {
  position: fixed;
  width: 100%;
  z-index: 9999;
-  transform: translate3d(0,0,0);
+  transform: translate3d(0, 0, 0);


Thanks for fixing all these style inconsistencies, but we'd prefer to keep them out of this PR as it makes it harder to review (could make another PR to address styling issues).

Removed this and one more.

yangshun · 2019-05-30T05:42:07Z

packages/docusaurus-1.x/lib/core/Head.js

@@ -172,6 +172,7 @@ class Head extends React.Component {
            }}
          />
        )}
+        <script src="/js/scrollSpy.js" />


This assumes a baseUrl of /, which is an incorrect assumption. See L186 where a similar script was added for the codetabs functionality.

yangshun · 2019-05-30T05:43:04Z

packages/docusaurus-1.x/lib/static/js/scrollSpy.js

+      return;
+    }
+    const bounding = item.getBoundingClientRect();
+    if (


I don't really understand what this entire block does, do you mind explaining?

Also it's really uncommon to have an empty if block. It'd be better to negate the condition and shift the else into the negated if condition.

Removed unnecessary code from here and also I've left a lot of comments and modified variable names to better understand what is going on. If it still doesn't make any sense I will be happy to explain it here.

I'm also trying to learn the chunk, here's my understanding, correct me if I'm wrong, and can consider the following comment?

(function scrollSpy() { const OFFSET = 10; let timer; let headingsCache; const findHeadings = function findHeadings() { return headingsCache || document.querySelectorAll('.toc-headings > li > a'); }; const onScroll = function onScroll() { if (timer) { // throttle return; } /** * On every call, try to find header right after <-- next header * the one whose content is on the current screen <-- highlight this */ timer = setTimeout(function() { timer = null; let activeNavFound = false; const headings = findHeadings(); // all of toc nav anchors for (let i = 0; i < headings.length; i++) { let currNavActive = !activeNavFound; /** * Enter the following check up only when an active nav header is not yet found * Then, check the bounding rectangle of the next header * The headers that are scrolled passed will have negative bounding rect top * So the first one with positive bounding rect top will be the nearest next header */ if (currNavActive && i < headings.length - 1) { const next = headings[i + 1].href.split('#')[1]; const nextHeader = document.getElementById(next); const top = nextHeader.getBoundingClientRect().top; currNavActive = top > OFFSET; } /** * Stop searching once a first such header is found, * this makes sure the highlighted header is the most current one */ if (currNavActive) { activeNavFound = true; headings[i].classList.add('active'); } else { headings[i].classList.remove('active'); } } }, 100); };

Yupp that's correct. I will update the comments. Thank you :)

…fy scrip load url

yangshun

Looks great, many thanks @tusharf5!

If you could try to add that functionality to v2 too that'll be nice

tusharf5 · 2019-05-30T16:06:02Z

Working on it. Will submit a PR soon 🙋‍♂️

tusharf5 requested a review from endiliey as a code owner May 28, 2019 22:36

facebook-github-bot added the CLA Signed Signed Facebook CLA label May 28, 2019

yangshun suggested changes May 29, 2019

View reviewed changes

endiliey assigned yangshun May 29, 2019

yangshun suggested changes May 29, 2019

View reviewed changes

yangshun suggested changes May 30, 2019

View reviewed changes

tusharf5 added 4 commits May 30, 2019 17:59

feat: highlight nav item in onPageNav ToC

58e0ec1

fix: use es5 in browser scripts, update documentation, css refactoring

f3e4685

fix: removes smooth scroll

3a6ad8c

fix: removes unnecessary codeblock , undo css formatting issues, modi…

71d1529

…fy scrip load url

tusharf5 force-pushed the feat-highlight-nav-item-on-pagenav-toc branch from 26bd209 to 71d1529 Compare May 30, 2019 12:31

fix: update scrollspy script comments

4a0e0ae

yangshun approved these changes May 30, 2019

View reviewed changes

yangshun merged commit 9514593 into facebook:master May 30, 2019

wgao19 mentioned this pull request Aug 12, 2019

[v2] use primary color for highlighted items in table of contents #1752

Closed

SantiagoGdaR mentioned this pull request Oct 27, 2019

feat(v2): highlight items in the table of content #1896

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: highlight nav item in onPageNav ToC #1524

feat: highlight nav item in onPageNav ToC #1524

tusharf5 commented May 28, 2019 •

edited

Loading

docusaurus-bot commented May 28, 2019 •

edited

Loading

docusaurus-bot commented May 28, 2019 •

edited

Loading

yangshun left a comment •

edited

Loading

endiliey commented May 29, 2019 •

edited

Loading

wgao19 commented May 29, 2019

tusharf5 commented May 29, 2019

endiliey commented May 29, 2019

endiliey commented May 29, 2019

tusharf5 commented May 29, 2019

yangshun left a comment

tusharf5 commented May 29, 2019

yangshun left a comment

yangshun May 30, 2019

tusharf5 May 30, 2019

yangshun May 30, 2019

tusharf5 May 30, 2019

yangshun May 30, 2019

tusharf5 May 30, 2019

yangshun May 30, 2019

tusharf5 May 30, 2019

wgao19 May 30, 2019

tusharf5 May 30, 2019

yangshun left a comment

tusharf5 commented May 30, 2019

feat: highlight nav item in onPageNav ToC #1524

feat: highlight nav item in onPageNav ToC #1524

Conversation

tusharf5 commented May 28, 2019 • edited Loading

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

Related PRs

docusaurus-bot commented May 28, 2019 • edited Loading

docusaurus-bot commented May 28, 2019 • edited Loading

yangshun left a comment • edited Loading

Choose a reason for hiding this comment

endiliey commented May 29, 2019 • edited Loading

wgao19 commented May 29, 2019

tusharf5 commented May 29, 2019

endiliey commented May 29, 2019

endiliey commented May 29, 2019

tusharf5 commented May 29, 2019

yangshun left a comment

Choose a reason for hiding this comment

tusharf5 commented May 29, 2019

yangshun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangshun left a comment

Choose a reason for hiding this comment

tusharf5 commented May 30, 2019

tusharf5 commented May 28, 2019 •

edited

Loading

docusaurus-bot commented May 28, 2019 •

edited

Loading

docusaurus-bot commented May 28, 2019 •

edited

Loading

yangshun left a comment •

edited

Loading

endiliey commented May 29, 2019 •

edited

Loading