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

Default nav order #236

Merged
merged 7 commits into from
Apr 27, 2020
Merged

Conversation

pdmosses
Copy link
Contributor

When nav_order is omitted, the order of nodes at each menu level (and in the auto-generated TOC) is now alphabetical by title, instead of random. Addresses issue #228.

When a non-alphabetic order is required at a particular level, it is recommended to set the nav_order for all its nodes. (If mixed, ordered nodes come before unordered nodes).

As at present, nav_order fields must have a uniform site-wide type: integers and strings cannot be mixed, otherwise Jekyll reports errors.

The implementation filters the ordered and unordered pages from site.html_pages, sorts them separately, and concatenates the resulting arrays. It has been tested by removing the nav_order fields from the children of the Utilities node, which results in alphabetic order at that menu level.

It should be straightforward to implement this change also for the recursive navigation feature proposed in PR #192.

When `nav_order` is omitted, the order of nodes at each menu level (and in the auto-generated TOC) is alphabetical by `title`, instead of random.
Any nodes with a specified `nav_order` precede all nodes at that level where it is omitted.
Note that `nav_order` fields must have a uniform site-ide type: integers and strings cannot be mixed, otherwise Jekyll reports errors.
The implementation filters the ordered and unordered pages from `site.html_pages`, sorts them separately, and concatenates the resulting arrays.
The `nav_order` fields are removed from the children of the Utilities node, which results in alphabetic order.
Added documentation of default nav order.
@pdmosses
Copy link
Contributor Author

pdmosses commented Oct 12, 2019

It should be straightforward to implement this change also for the recursive navigation feature proposed in PR #192.

For example, see https://pdmosses.github.io/test-nav/ where all nav_order fields have been removed: recursive navigation shows the pages in alphabetical order.

@thealjey
Copy link

#192 already does this btw

@pdmosses
Copy link
Contributor Author

@thealjey That is what I thought too, when I looked at your code. But when I tested it on my test-nav site with no explicit nav_order fields, it produced reverse alphabetic order! (That was when building the site locally; it's possible it would give a different result when uploaded to GH Pages, or when you test it locally.)

You have:

{%- assign hierarchy = site.html_pages | where_exp:"item", "item.nav_exclude != true"
     | sort:"title" | sort:"nav_order" | group_by:"parent" -%}

I suspect that the sort:"nav_order" filter doesn't preserve the current order when that field is nil. The sort filter has an option to control whether nil values are sorted before or after non-nil values, but I didn't find anything indicating that their original order is preserved. (The lack of detail in the Liquid documentation isn't helpful...)

My suggested implementation avoids sorting pages on nil fields, making it independent of their treatment by the filter (and ensuring I get the same result locally as on the uploaded site).

@thealjey
Copy link

hm, my pages all have a nav_order set to the same value - 1
so that's probably why I did not notice this problem
the reason I do the sorting twice, first by title and then by nav_order, is so that we have alphabetic sorting by default, and nav_order still having a higher priority when determining the order of pages

I think the sort filter simply treats nil as 0, and as such moves all of those pages to the beginning of the array, instead of keeping them in place

do you have a suggestion how to address this problem (while keeping the functionality)?

@pdmosses
Copy link
Contributor Author

I agree with your summary. Note however that the Liquid filters page states:

Optional arguments for hashes 1. property name 2. nils order (first or last).

The following appears to address the issue when using your recursive navigation code in https://github.com/pdmosses/test-nav:

{%- assign ordered_pages_list = site.html_pages | 
    where_exp:"item", "item.nav_exclude != true" |
    where_exp:"item", "item.nav_order != nil" | sort:"nav_order" -%}
{%- assign unordered_pages_list = site.html_pages |
    where_exp:"item", "item.nav_exclude != true" |
    where_exp:"item", "item.nav_order == nil" | sort:"title" -%}
{%- assign hierarchy = ordered_pages_list | concat: unordered_pages_list | group_by:"parent" -%}

I made related changes in some of the files in _includes.

@thealjey
Copy link

so, you simply want them at the end, rather than to preserve their alphabetical order?
that does not seem like a good solution to me
you're essentially doing the same thing the "last" parameter of sort is doing, just less efficiently
unless I'm missing something

@pdmosses
Copy link
Contributor Author

pdmosses commented Oct 17, 2019

My proposed fix ensures that pages at each menu level are sorted by title when no nav_order is specified. Your composition of sorting on title followed by sorting on possibly-nil nav_order doesn't, it seems.

Mixing pages with nav_order and pages with no nav_order at the same menu level is potentially confusing, but if someone does it (perhaps by accident) then I think the best is to put the pages without a nav_order all at the end, ordered by title.

Note also that when sorting on a mixture of title and nav_order, the nav_order fields would need to be strings, not numbers. To use an explicit nav_order to adjust the position of a page in an alphabetical-sorted menu level would require (all) nav_order fields to be strings, I think!

glennr pushed a commit to GrooveHQ/docs that referenced this pull request Feb 10, 2020
glennr pushed a commit to GrooveHQ/docs that referenced this pull request Feb 14, 2020
move ref doc gen back to API repo

generated ref docs using new md format

adds accordion style nav patch from just-the-docs/just-the-docs#252

splits out reference types

fix return type

orders TOC and children by name

retrofits just-the-docs/just-the-docs#236

fix ref types

connection types

allows great grandchildren

splits mutations into sub groups

add favicon

remove old reference htmls

move voyager to root

regen reference

update getting started

fref

fvoyager

fix voyager links

fix paths

connections grouped

fix breadcrumb, fix sidebar hierarchy

fref

snakecase all urls

fix field links for custom grouped objects

group return types

adds input type groupings

rm about

fix broken grouping for LabelInput (where we strip Input suffix)

fix toc and nav linking to other great grandchildren in reference tree
@alemsabic
Copy link

Changing »sort« with »sort_natural« would allow this theme to be used with the german language, where all nouns are capitalized and there should be no difference (when sorted alphabetically) between, let’s say »gut« (good) and »Gute« (the good).

sort : Sorts items in an array in case-sensitive order.

sort_natural : Sorts items in an array in case-insensitive order.

Wouldn’t it (even with the english language) make more sense to sort ( let’s say in a dictionary) in case-INSENSITIVE order?

By the way: Thanks a lot for this.

@pdmosses
Copy link
Contributor Author

@alemsabic thanks for the suggestion of using sort_natural to ignore case differences.

I agree that sort_natural should be the default. However, I'm inclined to add a configuration option to support case-sensitive order, since that might well be required (e.g.) in documentation for APIs.

@pmarsceill
Copy link
Collaborator

@alemsabic thanks for the suggestion of using sort_natural to ignore case differences.

I agree that sort_natural should be the default. However, I'm inclined to add a configuration option to support case-sensitive order, since that might well be required (e.g.) in documentation for APIs.

This sounds good to me, thanks for the work and thinking here @pdmosses @alemsabic and @alemsabic

@pdmosses
Copy link
Contributor Author

@pmarsceill I will try to update this PR tomorrow to incorporate the customisable sort order, and test (locally) that it works as intended.

How about the following names for use in the config file:

nav_sort: case_insensitive # default
# nav_sort: case_sensitive

I think that is clearer than Liquid's sort and sort_natural.

Also, I need to update the branch for this PR to the current release. I'm assuming that I can just click on Update branch here; let me know if that isn't the recommended procedure.

pdmosses and others added 3 commits April 25, 2020 09:52
Added `.jekyll-cache`
Added a configuration option to determine whether the sort order is case-sensitive.
The default is case-insensitive.

To test:
- open `/just-the-docs/docs/utilities/` in the browser,
  and check that the navigation links in `Utilities` are sorted alphabetically;
- in `docs/utilities/layout.md', change the preamble to `title: layout`,
  and check that the  links in `Utilities` are still sorted alphabetically;
- add `nav_sort: case_sensitive` in the configuration file,
  and check that the link to `layout` is now listed last under `Utilities`.
@pdmosses
Copy link
Contributor Author

A failing CI check appeared right after I clicked on Update branch on this page. I don't know how to correct that.

I have subsequently updated the PR with the suggested configuration option for case-sensitivity. To test:

  • open docs/utilities/ in the browser, and check that the navigation links in Utilities are sorted alphabetically;
  • in docs/utilities/layout.md, change the preamble to title: layout, and check that the links in Utilities are still sorted alphabetically;
  • add nav_sort: case_sensitive in the configuration file, and check that the link to layout is now listed last under Utilities.

@pmarsceill, could this be included in the next minor release? AFAIK, it is non-breaking.

@pmarsceill pmarsceill changed the base branch from master to v0.2.9 April 27, 2020 15:02
@pmarsceill pmarsceill merged commit b51decd into just-the-docs:v0.2.9 Apr 27, 2020
@pmarsceill pmarsceill mentioned this pull request Apr 27, 2020
@pdmosses
Copy link
Contributor Author

@alemsabic I've updated this PR to revert the default for nav_sort to sort. This is because sort_natural apparently changes the order for numbers to lexicographic (10 comes before 1!) and making that the default would probably surprise newcomers, as well as affecting the navigation of existing sites.

See also the comments about sort_numeric in this Liquid issue. In fact I run into the problem when the nav_order fields are simply numbers; perhaps sort_natural implicitly converts them to strings?

A potential workaround is to check whether the nav_order values on the site are all numbers or strings, and always use sort when they are numbers. But I'd prefer to leave implementing that to a later release.

@alemsabic
Copy link

@pdmosses Wow! Just read about it. Crazy you found this out by testing. I had about fifty pages on a German site and a Croatian site as well (with your proposed changes for the navigation plus sort_natural) and did not run into any issues (the reason of course being I did not use any numbers). Reading the conversation here it feels like they have forgotten about it; it seems to have been resolved but the changes have never been merged? Really? (Last comment: "What about this PR?" Hahaha.) Okay. Well … It’s probably a self-solver and will soon be implemented (as sort_numeric?). We, here, we won't forget it. No! :-))

@pdmosses
Copy link
Contributor Author

@alemsabic I fully agree :-) In fact I only discovered it accidentally (rather than by systematic testing): I was testing a different PR based on 0.2.9, and I added some test files, using numbers above 100 for the nav_order – expecting them to appear at the bottom of the nav menu. When they appeared at the top, I became suspicious of sort_natural.

BTW, I'm not familiar with the Ruby/Jekyll/Liquid code base, and I have difficulty finding the relevant sources. If you happen to know where to look for the source code for sort_natural, please post a link, as it would be good to pinpoint the cause of its apparent treatment of numbers as strings.

@pdmosses pdmosses deleted the default-nav-order branch August 11, 2020 06:45
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

5 participants