In-page quick-nav menu #741

nfagerlund · 2019-04-02T23:20:04Z

This adds a small drop-down menu at the top of each page. When clicked,
it reveals an in-page hierarchical TOC which you can use to jump directly to any
of the H2, H3, or H4 headers in the primary content. The TOC can be dismissed by
clicking outside it or by re-clicking the trigger.

gif:

desktop:

mobile:

This re-uses the SCSS from the top-nav dropdown, and uses a little bit of JQuery
to do the rest of the lifting.

armchairlinguist · 2019-04-02T23:22:45Z

This is SO EXTREMELY GOOD. :terraform-da: 💜
What do you think about calling it "Page Quick Nav" for a bit more clarity?

nfagerlund · 2019-04-02T23:24:05Z

"Page Quick Nav" sounds fine!

content/source/assets/javascripts/application.js

This commit adds a small drop-down menu at the top of each page. When clicked, it reveals an in-page hierarchical TOC which you can use to jump directly to any of the H2, H3, or H4 headers in the primary content. The TOC can be dismissed by clicking outside it or by re-clicking the trigger. This re-uses the SCSS from the top-nav dropdown, and uses a little bit of JQuery to do the rest of the lifting. I ruled out using Ruby to get the list of headers during the static build for a couple of reasons: - It would be significantly harder to accomplish. - It would be a dead-end investment, since we're moving away from the Middleman platform. - It risks a nasty effect on site build times and/or HTML munging code. (Previous experiments at using Nogokiri to safely extract what I want from the content HTML have not been good, but doing it with regexps can be even worse.) The extra event handlers to make click-based activation/deactivation work wouldn't be necessary if I'd just used hover-based activation like in the top navs, but that felt awkward once the TOC reached about the length of a screen.

Using margin-left was pushing the entire list item to the side, and on mobile widths, that would sometimes cross over the edge of the menu box. Padding doesn't move the rest of the element over, and the links should stay inside their li.

phinze

I love this! I'll hold off on ✅ since somebody qualified to review the code probably should give it a once-over, but I'm very +1 for it happening.

jescalan

Great idea here!

jescalan · 2019-04-03T21:43:57Z

content/source/assets/javascripts/application.js

+
+// On docs/content pages, add a hierarchical quick nav menu if there are any
+// H2/H3/H4 headers.
+document.addEventListener("turbolinks:load", function() {


Why put this in a different load function?

No good reason! I'll have it move in with the other one.

jescalan · 2019-04-03T21:44:52Z

content/source/assets/javascripts/application.js

+// On docs/content pages, add a hierarchical quick nav menu if there are any
+// H2/H3/H4 headers.
+document.addEventListener("turbolinks:load", function() {
+    "use strict";


any reason why this is only in this function rather than at the top level?

I thought it was only function scope?

If not, would it mess with anything being pulled in by the //= require statements?

Anyway, I'll defer to you!

It can be global as well, but that's a fair question about messing with other statements. Honestly I don't think you have any code here that violates strict mode anyway so it probably makes little difference, but usually i see people who use it putting it right at the top of files.

jescalan · 2019-04-03T21:45:31Z

content/source/assets/javascripts/application.js

+        );
+        var quickNavTrigger = $('#inner-quicknav #inner-quicknav-trigger');
+        var quickNav = $('#inner-quicknav > ul.dropdown');
+        headers.each( function(index, element) {


Is the space before the function on purpose?

JS is one of my more recently-acquired tongues, and sometimes I go overboard on trying to keep it readable for my future self. Probably not necessary.

jescalan · 2019-04-03T21:49:40Z

content/source/assets/javascripts/application.js

+        });
+        $('body').on('click', function() {
+            quickNav.removeClass('active');
+        });


Just a general question - since turbolinks prevents a full reload, does every page navigation transition continue stacking more and more of these event listeners? If so, maybe should we remove the event listeners somehow - didn't see a "before route change" event here

That's a great question. Lemme see if I can find out using the inspector tools.

I can't seem to do anything that causes doubled event listeners. After reading more, it looks like:

It at least doesn't cache event listeners. So they shouldn't get duplicated when someone navigates history w/ back and forward.

Turbolinks prevents a full reload, but it replaces the whole <body> element, right? That should cause it to discard any elements that have event listeners attached.

So, it seems like we should be good. But it's also possible I'm not seeing prod-like behavior in my preview... I'll build a complete copy of the site and test with that, as well.

Sure enough, even with a pre-built copy nothing seems to cause duplicated event listeners. The replacement of the entire <body> element and the fact that it doesn't cache event listeners for history navigation takes care of it, as expected.

Hmm, thinking more about this: Turbolinks WOULD probably cache the <span> and <ul> that we add to the div, since those are new DOM elements. But since we're adding them with .html("etc"), it's actually idempotent -- .html() replaces the current contents of that div.

Which means that on back/forward navigation, we aren't messing anything up, but we ARE doing a small amount of extra work by rebuilding the quick-nav.

I could throw that into some if statements so reattach the event listeners but don't rebuild the elements. But that might be premature optimization, since it doesn't really feel bad as-is.

Great investigation! It seems like, so long as when the element is removed no references to it remain, we have no potential for a memory leak here. I assume that turbolinks isn't holding on to internal references in this manner, and if it did, someone would have called it out a while ago, so we're probably good here.

alisdair

A couple of minor notes, but otherwise looks good to me!

alisdair · 2019-04-05T19:03:54Z

content/source/assets/javascripts/application.js

+    // On docs/content pages, add a hierarchical quick nav menu if there are any
+    // H2/H3/H4 headers.
+    // Check whether turbolinks cached the quick-nav elements:
+    if ($("#inner #inner-quicknav ul").length == 0) {


Is this ever non-zero, in your experience? I haven't tried it locally, but I thought turbolinks cached pages before calling turbolinks:load handlers, so I don't understand how it would ever save the generated quick-nav.

Oh, I might have misunderstood the timing on the cache! Yeah, I've never managed to make it be non-zero. I guess I might as well not test for that after all.

alisdair · 2019-04-05T19:22:23Z

content/source/assets/javascripts/application.js

+    });
+    $('body').on('click', function() {
+        quickNav.removeClass('active');
+    });


I think this event handler is going to be added again and again for every page transition. You might want to namespace it, and remove it first? $('body').off('click.quicknav'); $('body').on('click.quicknav', function() { …

Haha I had the same concern in my review above, but it seems as though if turbolinks replaces the entire body element and no references to the previous element remain, the event will be removed and garbage collected. It does still make me uncomfortable to see this though.

I don't think turbolinks replaces the body element, just its contents. For event handlers on all other elements I think we're safe, but I think body is a special case. I could be mistaken though!

Took a quick look in their docs and it says it does, although it might be worth verifying: https://github.com/turbolinks/turbolinks#navigating-with-turbolinks

Ah, because it's on <body> instead of a sub-element?

I tested this by having that handler just log something to the console, and navigated across a whole bunch of pages. But it just kept logging only once per click. So, I think that means it's not being re-added.

But if you think it's good practice to do the off/on thing, I can do it anyway!

lol sorry, I commented without reloading the page first. Well, anyway, I verified it. 😅

Add comments, remove a single-use variable, change an ES6 template string to concatenation.

nfagerlund · 2019-04-05T23:54:36Z

Ok! Got functionality approval, got JS approval, responded to review comments (ended up removing the cache check — it seems like it'll never be a cache hit, which means it's a useless DOM query)... I'm doing this.

nfagerlund commented Apr 2, 2019

View reviewed changes

content/source/assets/javascripts/application.js Outdated Show resolved Hide resolved

nfagerlund and others added 6 commits April 2, 2019 16:54

Suppress quick-nav if there are no h2/h3/h4s in content

4c50287

Rename nav trigger

41ac34c

Make color of SVG chevron match font

cc3f0d5

Add comments for JQuery-based content helpers

c434899

nfagerlund force-pushed the apr19_quick_nav branch from bbad408 to c434899 Compare April 2, 2019 23:55

nfagerlund requested review from mwickett, phinze and rkst April 3, 2019 00:07

phinze reviewed Apr 3, 2019

View reviewed changes

jescalan reviewed Apr 3, 2019

View reviewed changes

Set up all our minor widgets in one function

f8394eb

alisdair approved these changes Apr 5, 2019

View reviewed changes

Minor cleanups

a230200

Add comments, remove a single-use variable, change an ES6 template string to concatenation.

nfagerlund force-pushed the apr19_quick_nav branch from 5dabab6 to a230200 Compare April 5, 2019 23:51

nfagerlund merged commit a3a01d0 into master Apr 5, 2019

nfagerlund deleted the apr19_quick_nav branch April 5, 2019 23:56

nfagerlund mentioned this pull request Apr 6, 2019

website: Update nav sidebar to take advantage of new terraform.io features hashicorp/terraform-provider-aws#8224

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In-page quick-nav menu #741

In-page quick-nav menu #741

nfagerlund commented Apr 2, 2019 •

edited

Loading

armchairlinguist commented Apr 2, 2019

nfagerlund commented Apr 2, 2019

phinze left a comment

jescalan left a comment

jescalan Apr 3, 2019

nfagerlund Apr 3, 2019

jescalan Apr 3, 2019

nfagerlund Apr 3, 2019

jescalan Apr 5, 2019

jescalan Apr 3, 2019

nfagerlund Apr 3, 2019

jescalan Apr 3, 2019

nfagerlund Apr 3, 2019

nfagerlund Apr 3, 2019

nfagerlund Apr 4, 2019

jescalan Apr 5, 2019

alisdair left a comment

alisdair Apr 5, 2019

nfagerlund Apr 5, 2019

alisdair Apr 5, 2019

jescalan Apr 5, 2019

alisdair Apr 5, 2019

jescalan Apr 5, 2019

nfagerlund Apr 5, 2019

nfagerlund Apr 5, 2019

nfagerlund commented Apr 5, 2019

In-page quick-nav menu #741

In-page quick-nav menu #741

Conversation

nfagerlund commented Apr 2, 2019 • edited Loading

armchairlinguist commented Apr 2, 2019

nfagerlund commented Apr 2, 2019

phinze left a comment

Choose a reason for hiding this comment

jescalan 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alisdair 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

nfagerlund commented Apr 5, 2019

nfagerlund commented Apr 2, 2019 •

edited

Loading