Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi-level navigation #462

Closed
wants to merge 15 commits into from
Closed

Conversation

pdmosses
Copy link
Contributor

@pdmosses pdmosses commented Oct 15, 2020

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

A summary of the background for this PR, and an overview of the code, are available here.

The rec-nav-2 branch used for this PR is based on the latest an outdated release of the theme (v0.3.3, October 2020).

To test rec-nav-2 locally as a remote theme:

  • Specify in _config.yml:
    remote_theme: pdmosses/just-the-docs@rec-nav-2
    
  • Specify in Gemfile:
    gem "just-the-docs", github: "pdmosses/just-the-docs", branch: "rec-nav-2"
    
  • Run:
    bundle update
    bundle exec jekyll serve
    

The proposed updated documentation pages for the theme navigation structure are available here.

To serve the rec-nav-2 documentation pages locally and display the results of the regression tests:

  • Download the rec-nav-2 branch
  • Run:
    bundle update
    bundle exec jekyll serve --config _config.yml,_tests.yml
    

- Update `_config.yml` to add plugin `jekyll-include-cache` and option `nav_cache`.
- Update `just-the-docs.gemspec` to add `jekyll-include-cache`.
- Add `_nav_cache_false.yml` to disable caching (for profiling).
- Add `_include_tests.yml` to include `docs/tests/`.
- Replace `_includes/nav.html` by `_includes/nav/*.html`.
- Update `_layouts/default.html` to include `nav/{main,crumbs,toc}.html`.
Run jekyll with option `--config _config.yml,_include_tests.yml` to check unchanged.
- Major update for recursive navigation.
- Explain disambiguation by ancestor and section id.
- Move notes for users of previous versions to footnotes.
- Move regression tests from `docs/tests` to `_tests`.
- Update `_config.yml` and `_include_tests.yml`.
- Configure front matter default layout for tests.
- Simplify navigation for tests using recursive navigation.
- Add tests for title disambiguation.
Run jekyll with option `--config _config.yml,_include_tests.yml` to check.
The regression tests have been expanded, and the navigation hierarchy is deeper.
- Update the site title to show the fork branch name.
- Needs to be reverted if merged.
- Update `url` to fix the internal links on the published site.
- Needs to be reverted if merged.
@pdmosses pdmosses mentioned this pull request Oct 15, 2020
- Rename `_include_tests.yml` to `_tests.yml`.
- Fix the check that the `ancesstor` title is in the path by enclosing it in a pair of "^"s, and changing the extension of the path to append the "^".
- Add regression tests with overlapping titles to show that ancestors are now strictly checked using the separators.
To do:
- Titles are assumed not to contain "^", which is used to separate titles in paths. Replace "^" by newline, which cannot occur in a title.
- Check that the grandchildren with incorrect ancestor fields now appear.
- Change the separator used in navigiation paths from `^` to newline.
- Add regression tests to check that ancestor titles do not match prefixes or suffixes.
@pdmosses
Copy link
Contributor Author

The last three commits above remove the need for avoiding a special character (^) in titles, and add regression tests where a page title is a prefix or suffix of a potential ancestor title.

@pdmosses pdmosses linked an issue Oct 24, 2020 that may be closed by this pull request
@pdmosses pdmosses linked an issue Nov 18, 2020 that may be closed by this pull request
@jtebert
Copy link

jtebert commented Nov 19, 2020

From a quick test, I believe that this also fixes the issue I opened yesterday (#492) about incorrect breadcrumbs and table of contents in collections.

@pdmosses
Copy link
Contributor Author

@jtebert thanks for testing this PR with multiple collections – I will add them to the regression tests.

@pdmosses
Copy link
Contributor Author

pdmosses commented Nov 20, 2020

N.B. If further collections are added, the TOC/breadcrumbs links in the Tests collection will be incorrect, as reported in #492.
The added tests for multiple collections confirm that this fixes #492.

@pdmosses pdmosses requested a review from pmarsceill November 23, 2020 09:06
- Add `_config_dev.yml`
- Relax gem version constraints
- Invert a condition in the test for top-level pages
This avoids the navigation for non-collection pages disappearing when there are no child pages.
- Add `test_collection_1` and `test_collection_2`
- Update `_tests.yml` to include the extra collections
TOC/breadcrumbs correctly link to the same collection.
@pdmosses pdmosses mentioned this pull request Feb 25, 2021
@pyorot
Copy link

pyorot commented Mar 8, 2021

If I rocket-react then hopefully this gets reviewed sooner :D
I'd be interested to know the roadmap for the project, what the priorities are, and if it's still active; maybe I should @ the maintainer?
Also interested in if there's an example website based on this PR, and how ready for use it is. I was hoping it'd be merged before I needed it, but it's been 5 months since the last commit.

simonebortolin added a commit to simonebortolin/just-the-docs that referenced this pull request Dec 24, 2022
simonebortolin added a commit to simonebortolin/just-the-docs that referenced this pull request Dec 27, 2022
@pdmosses pdmosses mentioned this pull request Feb 9, 2023
@adfoster-r7
Copy link

adfoster-r7 commented Mar 7, 2023

I haven't investigated the specific performance bottlenecks; but I wondered if the performance could be improved with a custom opt-in _plugin. i.e. the slow parts could be handled directly in Ruby instead of liquid. It would have to be opt-in functionality though if the plugin worked - as I believe by default plugins aren't supported on Github's default gh-pages, but they're available in Github actions, folk's own CI pipelines, or local dev environments

@pdmosses
Copy link
Contributor Author

pdmosses commented Mar 8, 2023

@adfoster-r7 thanks for the suggestion. However, I'm not a Ruby programmer, so I wouldn't develop such a plugin myself.

I'd like to update this PR to use v0.4, then start investigating whether it still suffers from the reported performance issues. But that will have to wait until I get time for it (hopefully from late April).

@mattxwang
Copy link
Member

Just circling back - slowly looking to devote more time on bigger JtD projects (instead of just maintenance). Anything I can do to help get this PR up to speed?

@pdmosses
Copy link
Contributor Author

@mattxwang

Anything I can do to help get this PR up to speed?

I think it would be better to open a new PR for supporting multi-level navigation, rather than continuing this one. The conversation here has become too long for comfort. Having to scroll down through a lot of (mostly resolved) comments would surely discourage discussion and reviews.

Moreover, some sites may have been using this PR branch for the past year or two. To avoid the risk of breaking those sites, I suggest to leave the PR branch as it is now. We can also leave the PR itself open until a new PR for multi-level navigation has been approved and merged.

@peterchenadded and @adfoster-r7 have made some suggestions for reducing the build times for sites using my current implementation of multi-level navigation (which can be painfully long, even when using Jekyll 4). Version 0.4 of JtD significantly reduced the build times for some larger 3-level sites; it's conceivable that incorporating similar optimisations would also help with multi-level sites.

However, we might also consider a more radical redesign of how we specify navigation in JtD sites. I recently took a closer look at Hugo, which seems to be generally regarded as the fastest SSG currently available. My curiosity had been piqued by some (partly misleading) comments about slow builds with JtD on Jekyll Talk.

I also noticed a Hugo theme called Docsy, which supports several features that we have yet to implement in JtD – including multi-lingual sites, versioned docs, and multi-level navigation! Using the folder structure of the source files to specify the navigation structure (apart from the relative order of pages at the same level) might allow JtD to reduce multi-level build times significantly, as well as circumventing the current need of the present PR for specifying section or ancestor to disambiguate parent title resolution.

So instead of trying to "get this PR up to speed", I'd prefer us to open a fresh issue (or simply a new discussion) to decide how best to support multi-level navigation in JtD – paying special attention to build times – before opening a PR to implement that support. Would that be okay?

@adfoster-r7
Copy link

@pdmosses Whatever you think is the best approach to move forwards with things works for me - I'm happy to test any new structure/format for bugs or performance comparisons as well 👍

Just as an extra datapoint; docusaurus also relies on the folder structure without needing extra frontmatter to be present when generating its sites:

They do provide an escape hatch for specifying a slug to ignore the folder and generate a custom path though example

@peterchenadded
Copy link

If we are talking about performance, we need to also assess the underlying technology being used.

For example, are people actually using liquid in the markdown files?

If we instead use markdown-it an in browser js markdown converter and change to single page design, the pages will load much much faster.

We had actually gone down this path already and the build and page load performance is light and day for our growing 2.5k page site.

@adfoster-r7
Copy link

are people actually using liquid in the markdown files?

I can't speak for the majority; For our docs we definitely use liquid for generating the links to other pages (example. I imagine folk are using different liquid filters. For our docs we also register a custom liquid filter to use.

If we instead use markdown-it an in browser js markdown converter and change to single page design, the pages will load much much faster.

I believe that might impact SEO and search engine indexing, as well as potentially screen readers. In my experience markdown parsers all have different semantics too, so the in-browser rendering might behave differently to Jekyll's parser - which would introduce regressions in behavior

I imagine that could probably be added on as a separate feature from multi-level navigation support either way, but I'll defer to the experts here

@peterchenadded
Copy link

I can't speak for the majority; For our docs we definitely use liquid for generating the links to other pages (example. I imagine folk are using different liquid filters. For our docs we also register a custom liquid filter to use.

I think we can divide parts of the page that need liquid and parts that do not need it. There is no way to remove liquid for layout files which includes for example header, nav, footer and aux links you mentioned. The actual content shown probably does not need it and pure markdown syntax is sufficient.

I believe that might impact SEO and search engine indexing, as well as potentially screen readers. In my experience markdown parsers all have different semantics too, so the in-browser rendering might behave differently to Jekyll's parser - which would introduce regressions in behavior

True, but for us our site is private and SEO was not important. Though, we still have the full.md files and should still work with SEO, if we really wanted it to.

Your right about the rendering being different. But actually markdown-it has good number of extensions. So for example if you wanted number lines for your code, you just use the extension.

Agree there will be regressions and some features will change but if build speed and page load performance is of concern, moving to SPA and not using liquid where it is not needed should be considered.

I imagine that could probably be added on as a separate feature from multi-level navigation support either way, but I'll defer to the experts here

Yes, nav is a different area and definitely needs to be cached for bigger sites. Just following the structure of the file system was good enough for us, but many will probably want front matter to define different titles. It was the worse performing when profiling but after it is fixed it is almost mandatory to fix the other two issues.

Forgot to mention search, which makes the page unresponsive for a second as it tries to load in search-data.json on every page you visit. We actually had to setTimeout and chunk the loading of that file to make the UI responsive.

@mattxwang
Copy link
Member

Hi all, thanks for the lively discussion! Bigger picture, I think a new issue could be appropriate; I could also put some work in simply porting this PR over to main (on a new branch, to avoid disrupting this one).

Let me address a few quick conversation topics:

  • personally, I think rendering markdown client-side is significantly out of scope for this project. There are other solutions that work quite well already, and in those cases, you probably shouldn't be using Jekyll; it is intentionally a static-site generator. The "hybrid" approach that's being alluded to - code splitting and doing the equivalent of SSR/islands for SSG - is way out of scope of most Jekyll themes. I think that would be more appropriate for the modern JS stack for this, e.g. Astro/Next/11ty and associated tooling.
  • file-based navigation is certainly appealing! I will point out that we are somewhat limited by Jekyll itself and that our theme is intended to work out-of-the-box with GitHub Pages (not necessarily a bad thing); since file-based navigation is not first-class, we'd have to do some hackiness to get it to work. The tradeoff is that Hugo is arguably less portable (depends on who you ask). Docusaurus is becoming the gold standard for OSS docs (Stylellnt has been using it for a while, for example), so it definitely makes sense to look at what they're doing.
  • I do think an opt-in Ruby plugin is within the cards; pretty sure that we can have users opt-in with a _config.yml parameter and only load in the plugin/file at runtime (i.e. when compiling). This preserves backwards compatibility.

Of course, these are all my opinions, and I'm happy to hear out some other perspectives!

In terms of my personal roadmap as a maintainer, there are some prerequisites I'll do before I work on this:

After that, I'm happy to then dedicate my maintenance time to pushing this PR forward.

In the meantime, if someone would either like to:

  • fork this branch and open an up-to-date PR
  • or, experiment with any of the above suggestions (with the caveat that I may deem an approach out of the spirit of JtD)

feel free to do so - happy to review!

@pdmosses
Copy link
Contributor Author

pdmosses commented Jun 9, 2023

Bigger picture, I think a new issue could be appropriate; I could also put some work in simply porting this PR over to main (on a new branch, to avoid disrupting this one).

In any case, I think I first should move all the multi-level tests from this PR branch to (a branch of) just-the-doc-tests.

BTW, I'm currently using the combination-rec-nav branch of my fork of JtD to publish a draft proposal for multi-level navigation docs and some accompanying notes. The published website is clearly labeled as a fork of a particular branch.

Let me address a few quick conversation topics:

  • personally, I think rendering markdown client-side is significantly out of scope for this project.

I know nothing about it, and I wouldn't be able to contribute.

  • file-based navigation is certainly appealing! I will point out that we are somewhat limited by Jekyll itself and that our theme is intended to work out-of-the-box with GitHub Pages (not necessarily a bad thing); since file-based navigation is not first-class, we'd have to do some hackiness to get it to work. The tradeoff is that Hugo is arguably less portable (depends on who you ask). Docusaurus is becoming the gold standard for OSS docs (Stylellnt has been using it for a while, for example), so it definitely makes sense to look at what they're doing.

I've also looked at Material for MkDocs, which has already implemented many of the features that we intend to support in JtD (multi-level navigation, i18n, versioned docs, toggled between color schemes, …). It seems that the navigation hierarchy there can either be left implicit in the directory structure, using file names as titles – with optional front matter to override the URL and/or the title displayed in the navigation panel – or specified in a single YAML file that associates titles with file names in the intended order and nesting. Jekyll provides variables such as page.path, and I don't see why any hacking would be needed to implement the same idea in JtD. There's also Docsy Jekyll, a port of the Hugo theme Docsy.

  • I do think an opt-in Ruby plugin is within the cards; pretty sure that we can have users opt-in with a _config.yml parameter and only load in the plugin/file at runtime (i.e. when compiling). This preserves backwards compatibility.

I've never programmed in Ruby, and I wouldn't be able to contribute to developing such a plugin. If we'd need to maintain consistency between a plugin and a Liquid implementation of multi-level navigation, some other maintainers would need to take responsibility for that.

Of course, these are all my opinions, and I'm happy to hear out some other perspectives!

In terms of my personal roadmap as a maintainer, there are some prerequisites I'll do before I work on this:

After that, I'm happy to then dedicate my maintenance time to pushing this PR forward.

I have a pressing conference submission deadline at the end of June. After that, I should have time to participate in further discussion and potential implementation of multi-level navigation.

In the meantime, if someone would either like to:

  • fork this branch and open an up-to-date PR

I think we need an in-depth discussion of what we would like to implement before opening a new PR on this topic.

There's an open discussion #425 with the question "Navigation levels limitation?". My response is now outdated, but I think it would be easy enough to update it, as a starting point for a fresh discussion of our current ideas for multi-level navigation.

An alternative might be to revisit the open issue #219 on "Navigation doesn't work with grand-grand-children". However, the linear structure of an issue doesn't seem as convenient as a discussion with threads of comments on potential answers, and discussions have the possibility of sorting them by the date of latest activity.

@ragalgut
Copy link

ragalgut commented Dec 2, 2023

It would be fantastic if this went ahead. A recursive navigation.

@pdmosses
Copy link
Contributor Author

pdmosses commented Dec 3, 2023

@ragalgut Thanks for your comment!

PR #1397 should make it much easier to add recursive navigation (by reducing dependence on the parent, grand_parent, and has_children relations).

I expect to have time this month to investigate whether the directory structure could determine the navigation hierarchy (apart from the title and nav_order, which would still be specified in front-matter). I think that would have some significant benefits, although there might also be some drawbacks.

I suggest to continue the discussion in #1277, to make it more visible.

@didacrios
Copy link

Hi @pdmosses is there any consensus achieved related to this point? Seen the branch from the proposal far from the current main branch of the project and conflicts raised related to this https://just-the-docs.com/migration/#moved-include

pdmosses added a commit to pdmosses/just-the-docs that referenced this pull request Feb 24, 2024
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.
@pdmosses
Copy link
Contributor Author

This PR is to be superseded by #1431.

Users with websites built with the PR branch rec-nav-2 should consider migrating to the multi-level branch, as rec-nav-2 will not be updated, and may eventually be deleted.

@pdmosses
Copy link
Contributor Author

Hi @pdmosses is there any consensus achieved related to this point?

@didacrios PR #1431 generalises the latest JtD release to allow any number of navigation levels. However, we cannot claim that a consensus has been achieved, as there has not been any further discussion of the adopted approach.

pdmosses added a commit that referenced this pull request Aug 20, 2024
* Allow unlimited multi-level navigation

This PR supersedes #462.

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.

The implementation has been significantly simplified by the changes introduced in v0.7.0 of the theme.

* Detect cyclic parenthood

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).

* Add nav_error_report warning in main navigation

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.

* Cache site-nav with links to all pages

The extra cached site-nav is used for determining breadcrumbs and children navigation, which may involve pages that are excluded from the main navigation.

* Replace code for determining children by inclusion of components/nav/children.html

* Update CHANGELOG.md

---------

Co-authored-by: Matt Wang <matt@matthewwang.me>
@pdmosses
Copy link
Contributor Author

Superseded by #1431

@pdmosses pdmosses closed this Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment