Feature: Allow unlimited multi-level navigation #1431

pdmosses · 2024-02-24T21:47:36Z

This PR is intended to supersede both #192 and #462. It aims to fix #219, and to respond to the discussion in #425 and in the comments on #192.

A summary of the background for this PR is available here.¹

The only user-level difference from #462 is that disambiguation of parent pages has to use either grand_parent or ancestor titles: the somewhat unnatural section_id and in_section fields are not supported.

Note

Multi-level navigation is to be backwards-compatible with existing sites that use JtD v0.8.*.²

Tasks

Make auto-generated TOCs take disambiguation by ancestor fields into account.
Generate all levels of breadcrumbs for pages excluded from the main navigation.
Make all levels of navigation expandable when JS is disabled.
Optimise the implementation of breadcrumbs and auto-generated TOCs when using Jekyll 3 to build a large site with more than 100 excluded pages.
Move temporary changes and additions to the docs pages to a fresh PR.

Overview of changes and additions

_includes/components/breadcrumbs:
- update to multilevel
_includes/components/children_nav:
- update to multi-level
_includes/components/nav/*:
- replace uses of _includes/components/nav.html
_includes/components/site_nav:
- add a boolean parameter for optionally including links to pages that are excluded from the main navigation
_layouts/default:
- remove test of has_children

To simplify review and testing, the temporary docs-related changes have been moved to PR #1440.

The original overview of the implementation is outdated – the implementation has been significantly simplified by the navigation changes introduced in v0.7.0 of the theme. ↩
Sites that currently use modified copies of the changed files will need to update those copies. ↩

This PR is intended to supersede just-the-docs#462. The only user-level difference from just-the-docs#462 is that disambiguation of parent pages has to use either `grand_parent` or `ancestor` titles: the somewhat unnatural `section_id` and `in_section` fields are not supported. The implementation has been significantly simplified by the changes introduced in v0.7.0 of the theme. This initial draft has not yet been rigorously tested nor profiled.

Unclear to me why adding this gem causes CI failures.

pdmosses · 2024-02-25T13:18:43Z

HTML Proofer failed:

For the Links > External check, the following failures were found:

At _site/docs/navigation-structure/recursion/X/index.html:1:

External link https://just-the-docs.com/docs/navigation-structure/recursion/X/ failed (status code 404)

...

When building the theme docs locally, the following link appears in the head:

<script type="application/ld+json">
  {..., "url":"http://localhost:.../docs/navigation-structure/recursion/X/"}
</script>

It seems that HTML Proofer assumes the site is being built at https://just-the-docs.com/. But as the page is a new one, it doesn't yet exist in the theme docs.

Is there a work-around for avoiding such failures when adding new files in PRs?

A page should not have a parent or ancestor with the same title. If it does, the location of the repeated link is marked by ∞, to facilitate debugging the navigation (and an unbounded loop leading to a build exception is avoided). It might be preferable to emit a warning in the Jekyll build log, but that doesn't appear to be supported by Liquid/Jekyll tags.

Reuses `nav_breadcrumbs`.

In Jekyll 3, Liquid’s binary `and` and `or` operations cannot be used in the `where_exp` filter...

Avoid diffs from sites built with v0.8.0

For backward compatibility with 3-level sites

This is for backwards compatibility with sites that use `has_children: false` to suppress navigation links to child pages.

pdmosses · 2024-02-28T22:17:03Z

This initial draft has not yet been rigorously tested

The current version of the draft appears to be backwards compatible with v0.8.0: it produces essentially the same HTML when building the Just the Docs Tests site, based on running the following command after building _site-0.8.0 and _site-pr:¹

just-the-docs-tests: diff -rw -x "*.js*" -x "*.map" -I "published_time" -I "dateModified" -I "<meta property=" -I "<link rel=" -I "@context" _site-pr _site-0.8.0 | more
diff -rw -x *.js* -x *.map -I published_time -I dateModified -I <meta property= -I <link rel= -I @context _site-pr/index.html _site-0.8.0/index.html
303,305c303
<     <div class="language-plaintext highlighter-rouge"><div class="highlight"><pre class="highlight"><code>remote: https://github.com/pdmosses/just-the-docs.git
< revision: a649a473b591fa8b64c7ce972d949c9147758155
< ref: multi-level
---
>     <div class="language-plaintext highlighter-rouge"><div class="highlight"><pre class="highlight"><code>remote: ../just-the-docs-v0.8.0

… nor profiled.

Initial profiling indicates that the current draft is up to 10% slower than v0.8.0 with Jekyll 4 on larger 3-level sites.

With Jekyll 3, the implementation of the auto-generated TOCs is currently quite inefficient on one large 3-level site. It should be possible to fix that for non-excluded pages, by deconstructing the cached site-nav.

Annoyingly, the line breaks in the HTMl for the navigation panel are non-uniform when building with v0.8.0, but uniform with the current draft. To avoid them generating spurious diffs for all files, _site-0.8.0 was built with a local clone of v0.8.0 where the line breaks had been adjusted. ↩

mattxwang · 2024-02-28T23:18:18Z

Thanks for opening this PR up! I will try to take a more in-depth look in a bit. But, quickly:

Is there a work-around for avoiding such failures when adding new files in PRs?

My off-the-cuff suggestion is to replace the site.url when running HTMLProofer to make it search locally instead of at just-the-docs.com, but I'm not sure how this interplays with the jekyll-seo-tag plugin. As a stopgap, we could also have it check the deploy preview for just this PR (without needing to disable/ignore the check as a whole).

I can take a quick look at this!

Takes account of ancestors. To test, add to 404.html front matter: ```yaml parent: T grand_parent: S ancestor: Y ``` Then check that 404.html has the correct breadcrumbs.

When activated by `nav_error_report: true` in `_config.yml`, displays warnings about pages with the same title as their parent page or an ancestral page.

The breadcrumbs for excluded pages were incorrect in previous commits, due to interference between assignments in recursive inclusions of ancestry.html.

pdmosses · 2024-03-10T09:05:48Z

I can take a quick look at this!

No rush: I intend to remove all the new files before requesting a review. That will avoid the HTML-Proofer issue.

It appears that: - `nav_levels[1] == nil` holds only when not `site_nav contains nav_list_link`; - `nav_levels[2] == nil and nav_levels[3]` never holds.

* Generate rules for any number of levels * Improve layout of generated rules * Improve layout of Liquid tags

pdmosses · 2024-03-12T19:34:39Z

On a large site where 100s of pages are excluded from the main navigation, the current multi-level implementation of breadcrumbs is too inefficient. A significantly more efficient implementation will have to be developed.

Revert to the original code for nav/ancestry. The previous draft increased the build time 5x with Jekyll 3 for a site with about 550 pages, of which 150 are excluded from the main navigation.

pdmosses · 2024-03-13T21:47:42Z

The latest commit has improved the efficiency of the implementation of breadcrumbs for excluded patges.

However, when building a site with about 550 pages – of which about 150 are (implicitly. or explicitly) excluded from the main navigation – the updated profiling results show that the rendering of the breadcrumbs and TOCs takes roughly 35 seconds each with this PR and Jekyll 3. This is significantly longer than when building the same site with v0.8.1 and Jekyll 3, where rendering the breadcrumbs takes about 18 seconds, and the TOCs only 2 seconds. (With this PR and Jekyll 4, the breadcrumbs and TOCs take 2–3 seconds each, which seems reasonable.)

pdmosses · 2024-03-14T10:19:12Z

The latest commit has improved the efficiency of the implementation of breadcrumbs for excluded patges.

An alternative would be to ensure that the site_nav includes links to all pages. The display of links to pages that are supposed to be excluded from the main navigation would then need to be suppressed using CSS.

when building a site with about 550 pages – of which about 150 are (implicitly. or explicitly) excluded from the main navigation – the updated profiling results show that the rendering of the breadcrumbs and TOCs takes roughly 35 seconds each with this PR and Jekyll 3. This is significantly longer than when building the same site with v0.8.1 and Jekyll 3,

Locally profiling a preliminary test of this alternative approach indicates that it would reduce the Jekyll 3 build time from 105 seconds to 55 seconds (the same as the v0.8.1 build time). The inclusion of the extra 150 links in site_nav significantly increases the size of all pages, from about 32MB to 42MB, but it should be possible to avoid that by caching two versions of the site_nav.

The extra cached site-nav is used for determining breadcrumbs and children navigation, which may involve pages that are excluded from the main navigation. The inefficient code previously used for excluded pages is now redundant, and to be removed in a subsequent commit.

pdmosses · 2024-03-15T11:27:06Z

... by caching two versions of the site_nav.

After that change, the updated profiling results for this PR are close to those for v0.8.1.

Replace code for determining children by inclusion of components/nav/children.html Profiling locally, this doesn't appear to lose efficiency.

mattxwang

I apologize for the extraordinarily long turn around time for this PR - I'm realizing I am continually grossly underestimating how challenging my new job can be (at times).

I've done a first pass over the general logic of this PR, the motivation (thank you for the great summary document and comparison to #192, as well as an explanation of the title-based assumptions), and briefly looked at the performance results. I will admit that I have not yet looked at edge/corner cases nor thought about how this could adversely impact legacy sites. However, broadly speaking, I'm pretty supportive of this change and would like to merge it in.

I will do a more thorough look "when I'm free" (which is ideally, this or next weekend - have some time on a plane to review code). I've left a handful of broad implementation questions & little things that were unclear to me on first read; none of these necessitate response until my next review, but let me know if you have any thoughts!

mattxwang · 2024-04-25T09:36:30Z

_includes/css/activation.scss.liquid

+    {%- for i in (2..nav_page_level) %},
+    {{ activation_collection_prefix }} > li.nav-list-item:nth-child({{ nav_levels[1] }}) >
+    {%- for j in (2..i) %} ul.nav-list > li.nav-list-item:nth-child({{ nav_levels[j] }}) >
+    {%- endfor %} ul.nav-list
+    {%- endfor %} {


Selectors that look like this are linear in the size of nav_page_level, right? I am wondering at what point CSS selector performance will become an issue. My guess is that most of our existing client sites won't have issues here (since they've been artificially limited to a depth of 2-3), but I'm curious how large sites with many more levels of nesting will do. I don't think that should be a barrier to implementing this change, but may be a future roadblock.

(I'm also cognizant that backwards-compatibility makes it even more challenging to implement this change well)

I'm curious how large sites with many more levels of nesting will do.

I think that sites with large numbers of pages tend to have high average branching. For 1000 pages with average branching 10, the average depth would probably be about 3. Even for a very slim site with 1000 pages and binary branching, the average depth wouldn't be much more than 10. Do you know of any sites with more than 10 navigation levels?

I know nothing about CSS selector efficiency. Would 10 levels of selectors make a website significantly less responsive? Note that most of the generated selectors are for direct descendants (using >) which ought to be quick to test.

In any case, the generated page-dependent CSS rules are disabled when JS is active, so the vast majority of visitors would never be affected by any inefficiency with the selectors.

(I'm also cognizant that backwards-compatibility makes it even more challenging to implement this change well)

Removing the restriction to 3 levels actually simplified the navigation link generation. I think the only tricky bit with backwards-compatibility was to treat a missing grand_parent field as indicating that the page indeed has no grandparent page when disambiguating parent pages.

mattxwang · 2024-04-25T09:37:00Z

_includes/css/activation.scss.liquid

+  {%- comment -%}
+    not site_nav contains nav_list_link
+  {%- endcomment -%}


I may be missing something, but what does this comment mean?

It merely asserts the condition that holds when the else branch is used: it is the negation of the condition tested on line 46:

{%- if site_nav contains nav_list_link -%}

mattxwang · 2024-04-25T09:50:25Z

_includes/components/nav/links.html

+    {%- capture nav_error_report -%}
+      <blockquote class="warning">
+        A page has the same title as its parent page or one of its ancestral pages!<br>
+        This causes an incorrect link in the main navigation panel.<br>
+        Page title: <code>{{ node.title }}</code>, location: <code>{{ node.path }}</code>.
+      </blockquote>
+    {%- endcapture -%}


Do you think this would also be helpful to log in the JS console?

mattxwang · 2024-04-25T09:56:18Z

_includes/components/nav/links.html

+
+    <li class="nav-list-item">
+      {%- if nav_children.size >= 1 -%}
+      <button class="nav-list-expander btn-reset" aria-label="toggle items in {{ node.title }} category" aria-pressed="false">


I am just realizing now that we may not be properly setting this aria-pressed value when the expander is open by default (looking at the JS, ariaPressed is only set by an event listener - so never fired on first load). This is not a regression in this PR so this doesn't need to be addressed right now.

pdmosses mentioned this pull request Feb 24, 2024

Multi-level navigation #462

Open

pdmosses added 5 commits February 24, 2024 23:02

Update Gemfile.lock

4919cb1

Update Gemfile

f9db7fd

Unclear to me why adding this gem causes CI failures.

Add jekyll-default-layout in gemspec

7b99d02

Correct link filename case

1a02812

Add the jekyll-default-layout gem in fixtures

9e1fce3

pdmosses added 12 commits February 25, 2024 19:13

Remove has_children test

5c7d2d1

Add to comment

8dc6153

Replace condition by size test

47a5c79

Filter child pages for ancestors

25e412e

Reuses `nav_breadcrumbs`.

Avoid Jekyll 4 syntax

6cdc654

In Jekyll 3, Liquid’s binary `and` and `or` operations cannot be used in the `where_exp` filter...

Adjust generated line breaks

5da5c87

Avoid diffs from sites built with v0.8.0

Filter children

c4f762a

For backward compatibility with 3-level sites

Filter children

eecba73

For backward compatibility with 3-level sites

Fix child_nav_order

f50c207

Add quick check for no children

2ee3af5

Support has_children: false

a649a47

This is for backwards compatibility with sites that use `has_children: false` to suppress navigation links to child pages.

pdmosses added 5 commits March 4, 2024 21:02

Fix breadcrumbs for excluded pages

c31d6e1

Takes account of ancestors. To test, add to 404.html front matter: ```yaml parent: T grand_parent: S ancestor: Y ``` Then check that 404.html has the correct breadcrumbs.

Add nav_error_report warning in main navigation

c832248

When activated by `nav_error_report: true` in `_config.yml`, displays warnings about pages with the same title as their parent page or an ancestral page.

Fix breadcrumbs for excluded pages

9be113b

The breadcrumbs for excluded pages were incorrect in previous commits, due to interference between assignments in recursive inclusions of ancestry.html.

Merge remote-tracking branch 'upstream/main' into multi-level

c825c8b

Add nav_error_report setting

2b6d919

pdmosses changed the title ~~Allow unlimited multi-level navigation~~ Feature: Allow unlimited multi-level navigation Mar 10, 2024

Control flow simplified

6061496

It appears that: - `nav_levels[1] == nil` holds only when not `site_nav contains nav_list_link`; - `nav_levels[2] == nil and nav_levels[3]` never holds.

Update to multi-level

ce52b12

* Generate rules for any number of levels * Improve layout of generated rules * Improve layout of Liquid tags

Optimise breadcrumbs for excluded pages

53b47c3

Revert to the original code for nav/ancestry. The previous draft increased the build time 5x with Jekyll 3 for a site with about 550 pages, of which 150 are excluded from the main navigation.

pdmosses added 5 commits March 15, 2024 13:10

Remove redundant code

7deaf18

Avoid code duplication

f454aee

Replace code for determining children by inclusion of components/nav/children.html Profiling locally, this doesn't appear to lose efficiency.

Clarify comments

00b182d

Remove docs changes

bf2230c

Update Gemfile.lock

c744937

pdmosses mentioned this pull request Mar 17, 2024

Docs: Allow unlimited multi-level navigation #1440

Open

pdmosses marked this pull request as ready for review March 17, 2024 21:49

pdmosses requested a review from mattxwang March 17, 2024 21:49

pdmosses mentioned this pull request Apr 20, 2024

Add nav_enabled variables for more customizable and feature-complete minimal layouts #1441

Merged

Merge branch 'main' into multi-level

61a97d0

mattxwang reviewed Apr 25, 2024

View reviewed changes

pdmosses mentioned this pull request Apr 26, 2024

Parent page has_children should not require child pages parent setting #1302

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Allow unlimited multi-level navigation #1431

Feature: Allow unlimited multi-level navigation #1431

pdmosses commented Feb 24, 2024 •

edited

pdmosses commented Feb 25, 2024 •

edited

pdmosses commented Feb 28, 2024

mattxwang commented Feb 28, 2024

pdmosses commented Mar 10, 2024

pdmosses commented Mar 12, 2024

pdmosses commented Mar 13, 2024

pdmosses commented Mar 14, 2024

pdmosses commented Mar 15, 2024

mattxwang left a comment

mattxwang Apr 25, 2024

pdmosses Apr 25, 2024

mattxwang Apr 25, 2024

pdmosses Apr 25, 2024

mattxwang Apr 25, 2024

mattxwang Apr 25, 2024

Feature: Allow unlimited multi-level navigation #1431

Are you sure you want to change the base?

Feature: Allow unlimited multi-level navigation #1431

Conversation

pdmosses commented Feb 24, 2024 • edited

Tasks

Overview of changes and additions

Footnotes

pdmosses commented Feb 25, 2024 • edited

pdmosses commented Feb 28, 2024

Footnotes

mattxwang commented Feb 28, 2024

pdmosses commented Mar 10, 2024

pdmosses commented Mar 12, 2024

pdmosses commented Mar 13, 2024

pdmosses commented Mar 14, 2024

pdmosses commented Mar 15, 2024

mattxwang left a comment

Choose a reason for hiding this comment

mattxwang Apr 25, 2024

Choose a reason for hiding this comment

pdmosses Apr 25, 2024

Choose a reason for hiding this comment

mattxwang Apr 25, 2024

Choose a reason for hiding this comment

pdmosses Apr 25, 2024

Choose a reason for hiding this comment

mattxwang Apr 25, 2024

Choose a reason for hiding this comment

mattxwang Apr 25, 2024

Choose a reason for hiding this comment

pdmosses commented Feb 24, 2024 •

edited

pdmosses commented Feb 25, 2024 •

edited