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

Safe page sorting #411

Merged
merged 23 commits into from Sep 11, 2020
Merged

Safe page sorting #411

merged 23 commits into from Sep 11, 2020

Conversation

pdmosses
Copy link
Contributor

Currently, the values of nav_order fields are required to be uniformly all numbers or all strings. If different types of values are (accidentally) used, building fails, with a somewhat confusing error report (e.g., see issue #406).

The changes to _includes/nav.html eliminate the possibility of such build failures arising. They also prevent nav_sort: case_insensitive affecting the ordering of numerical nav_order parameters (where 10 came before 2!). And they make it possible to use numbers to order some pages, and strings to order others.

The PR adds tests for various navigation features. These are excluded by default, and can be activated by commenting-out a line in _config.yml. (The tests inherently involve mixtures of different types of values for nav_order fields, so they cannot be used with previous versions of _includes/nav.html.)

This PR incorporates the bug fixes of PR #407 regarding nav_exclude and pages with numeric titles.

pdmosses and others added 17 commits November 16, 2019 11:44
Display of grandchildren links in the navigation is now delayed until their parent is selected.

To test, select the `Grandchildren test` node. Only the direct children should appear. Selecting one of them then shows its children.
Trying to get the navigation to remain in the forked site
Trying to get the navigation to remain in the forked site
Trying to get the navigation to remain in the forked site
Trying to get the navigation to remain in the forked site
Added `.jekyll-cache`
Pages with `nav_exclude: true` were included when sorting on `title` or `nav_order`. That could cause build failures when the type of value of the field differs from that on other pages, as reported in just-the-docs#406.

Pages with `nav_exclude: true` or no `title` are never displayed in the navigation, so removing them from `pages_list` cannot break existing sites. This change also allows the removal of some tests in the code. (The indentation of the code should now be adjusted, but that has been deferred, to restrict the size of the diff for review.)

For testing, the title of `404.html` has been changed to the number `404`,  the page `docs/untitled-test.md`  has been added, and `nav_sort_order` has been set to `case_sensitive`. Those updates give build failures with the current version of `_includes/nav.html`, but not after the suggested changes.

It will still be possible for build failures to occur due to sorting fields of *non-excluded* pages with differing types of values (e.g., `nav_order`a mixture of numbers and strings). To make the code completely safe will require relatively complicated changes,.
The values of `title` and `nav_order` can be numbers or strings.
Jekyll gives build failures when sorting on mixtures of different types,
so numbers and strings need to be sorted separately.

Here, numbers are sorted by their values, and come before all strings.
An omitted `nav_order` value is equivalent to the page's `title` value
(except that a numerical `title` value is treated as a string).

The case-sensitivity of string sorting is determined by `site.nav_sort`.
Indentation adjusted
Adjusted the documentation to explain how mixtures of numbers and strings are treated by `nav_order`.
Fixed conversion of numeric titles to strings.
See the change to `_config.yml` for how to activate the tests.
@pdmosses
Copy link
Contributor Author

@pmarsceill the Liquid code in _includes/nav.html is unfortunately quite complicated; suggestions for improvement are welcome! I hope the accompanying comments provide sufficient explanation, but don't hesitate to query anything that seems unmotivated.

@pdmosses pdmosses changed the title Nav sorting Safe page sorting Aug 14, 2020
Copy link
Collaborator

@pmarsceill pmarsceill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing... The tests are a great idea.

@pmarsceill pmarsceill changed the base branch from master to v0.3.2 September 11, 2020 15:54
@pmarsceill
Copy link
Collaborator

pmarsceill commented Sep 11, 2020

@pdmosses could you resolve the conflicting file from the 0.3.2 branch and I will get it merged in?

@pdmosses
Copy link
Contributor Author

@pmarsceill I'll take a look now, and let you know.

Apparetnly Jekyll's `include` config option cannot be used to override an `exclude`, so activating `docs/tests/` requires commenting-out that line in the `exclude` list.
Reinstated the collapsible TOC at the top, to support the reference to it right at the end of the file. (The `TOC` feature can only be used once per page, so this is the only way of illustrating the rendering of the collapsible TOC in the docs.)
_config.yml Outdated Show resolved Hide resolved
url corrected
doc for turning on kramdown linenos globally corrected/
@pdmosses
Copy link
Contributor Author

@pmarsceill I've updated _includes/nav.html with the major changes regarding sorting and exclusion, and corrected the url config...

I've also updated /docs/tests/index.md with corrected instructions for editing _config.yml to activate the tests. The tests produce the intended results for me locally, but it would be good if you could check at your end. After activating the tests, the extra navigation items that appear should be consistent with all the points listed in Tests: Navigation. Let's switch to the neater technique you suggested in the next version.

And finally, I've reinstated the collapsible TOC at the top of the Navigation Structure page, to justify the reference to it in the last line of that page. If you really don't want the TOC at the top to be collapsible and revert that commit, the following sentence at the end will need adjusting or removing:

The result is shown at [the top of this page](#navigation-structure) (`{:toc}` can be used only once on each page).

@pdmosses
Copy link
Contributor Author

@pmarsceill the commit option on the requested change disappeared due to my independent commit, but the change request is still there... I hope it doesn't block merging. Apologies for my lack of expertise with updating PRs.

@pmarsceill pmarsceill merged commit 1c654ba into just-the-docs:v0.3.2 Sep 11, 2020
@pmarsceill pmarsceill mentioned this pull request Sep 14, 2020
@pdmosses pdmosses deleted the nav-sorting branch September 29, 2020 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants