-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Improve breadcrumb behaviour in disambiguating multiple grand_parents
#477
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
Improve breadcrumb behaviour in disambiguating multiple grand_parents
#477
Conversation
|
@AdityaTiwari2102 Thank you for reporting this bug with the breadcrumbs list, where a link on a grandchild page can point to the wrong parent page. Thank you also for the proposed change, which indeed fixes the bug. BTW, the theme already has a regression test that exhibits the bug. See |
pdmosses
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regression tests for disambiguation confirm the reported bug: the parent breadcrumb on http://localhost:4000/just-the-docs/docs/tests/navigation/disambiguation/dca/ incorrectly links to http://localhost:4000/just-the-docs/docs/tests/navigation/disambiguation/cb/
The proposed change makes it link correctly to http://localhost:4000/just-the-docs/docs/tests/navigation/disambiguation/ca/ and does not affect the other breadcrumb links.
|
@pdmosses - I can remove the regression test for disambiguation with this PR as well then, right? |
|
@AdityaTiwari2102 The regression test for disambiguation should not be removed, as it will be needed to check that future proposed changes do not affect this feature. The test should also be useful for those who change the implementation of navigation when customising the theme. The regression tests are excluded by default when building the theme documentation pages, so their existence does not affect the build efficiency. Ideally, building the theme with the tests included would produce error reports if the results are not as expected. Currently, however, the expected test results are not explicit, and we have to make visual checks of the pages produced by the tests. That is quite tedious, and errors can get overlooked. In the case of the breadcrumb issue, the breadcrumbs on the test pages all had the correct titles, but I neglected to check that those titles all linked to the correct pages. |
|
@pdmosses - Understood. Thanks for being patient. Is it possible for you to merge the PR? |
No: @pmarsceill does all the merging in preparation for new releases. I don't have sufficient experience with Git to do it myself. Usually, it's easy enough to use a PR branch as a remote theme while waiting for it to be merged in a future release. I tried with the following in my remote_theme: AdityaTiwari2102/just-the-docs@refactor/breadcrumb
plugins:
- jekyll-feed
- jekyll-remote-themeand this in my gem "jekyll-remote-theme"
gem "just-the-docs", github: "AdityaTiwari2102/just-the-docs", branch: "refactor/breadcrumb"The But when jekyll tried to build the site I got: I haven't seen that before. The only thing that seems unusual about your branch is that its name includes a |
When configured for more than one collection, v0.3.2 produces empty TOCs of children in all but the last collection, due to referencing the overwritten `pages_list` in `_layouts/default.html`. When the same parent title is used in different collections, v0.3.2 can also produce a link to the wrong collection. Moreover, v0.3.3 can also produce incorrect breadcrumb links; see issue just-the-docs#492. - Move the breadcrumbs link variable assignments from `_layouts/default.html` to `_includes/nav.html`, and make it conditional on `page` being in the current collection. - Correct the condition for assignment to `second_level_url`, as in just-the-docs#477. - Change the variable used for the TOC from `children_list` to `toc_list`, move its assignment from `_layouts/default.html` to `_includes/nav.html`, and make it conditional on `page` being in the current collection. - Add regression tests for TOC/breadcrumb links in multiple collections, with configuration commented-out. With the v0.3.3 regression tests active, `diff -r -q` on `_site/docs` reports that only `_site/docs/tests/navigation/disambiguation/dca/index.html` differs, which is due to the breadcrumb correction there.
- Fix a bug with the breadcrumbs list, where a link on a grandchild page can point to the wrong parent page, following the solution suggested by @AdityaTiwari2102 in just-the-docs#477
Manually merge the relevant changes from PR just-the-docs#494, namely: When configured for more than one collection, v0.3.2 produces empty TOCs of children in all but the last collection, due to referencing the overwritten `pages_list` in `_layouts/default.html`. When the same parent title is used in different collections, v0.3.2 can also produce a link to the wrong collection. Moreover, v0.3.3 can also produce incorrect breadcrumb links; see issue just-the-docs#492. - Move the breadcrumbs link variable assignments from `_layouts/default.html` to `_includes/nav.html`, and make it conditional on `page` being in the current collection. - Correct the condition for assignment to `second_level_url`, as in just-the-docs#477. - Change the variable used for the TOC from `children_list` to `toc_list`, move its assignment from `_layouts/default.html` to `_includes/nav.html`, and make it conditional on `page` being in the current collection.
|
Hi @AdityaTiwari2102, thanks for your contribution, and I'm sorry we're only getting to reviewing this now. I've set the target to |
|
@AdityaTiwari2102 In release candidate v0.4.0, the Liquid code changed by this PR has been moved to just-the-docs/_includes/nav.html Line 120 in 7a3c370
Please let me know if I got it wrong. BTW, the regression tests have all been removed in v0.4.0. They are to reappear in a separate repository that uses Just the Docs as a remote theme, and to be checked before v0.4.0 is released. In the meantime, a copy of the tests is available at https://github.com/pdmosses/just-the-docs/tree/combination-with-tests/_tests. |
Hey @pdmosses @mattxwang, Sorry for the delay in response. I have tested it on my application with the change and it seems to be working. I will remove it from |
grand_parents
|
@pdmosses could I defer one last review to you on this? If not, I can do it before |
|
@mattxwang this PR changes 0 files, so nothing to review. I should have added @AdityaTiwari2102 as a co-author of the relevant commit in the merged #578; I don't know how to do that retrospectively. |
Oh, so sorry - I was triaging all these PRs/issues late last night and this slipped my mind. Apologies for the ping (and thanks for your quick response elsewhere).
Hm, I'm not sure if we can do that without force-pushing unfortunately. We can make a manual note to add some attribution in our changelog! |
Extension of PR just-the-docs#977 to v0.4.0.rc1: - Make callouts `loud`. - Replace a warning paragraph by a callout. - Use a reference link instead of an explicit url for @pmarsceill . - Add link reference definitions for PRs and new contributors in v0.4.0.rc1. - Replace inline urls by shortcut link references. The changes to replace URLs by link references would be too tedious and error-prone to do completely manually for v0.4.0.rc1. The following regexp replacements were applied to the sections of v0.4.0.rc1, using Atom: - Copy a section of changes. - In the copy, replace `.*(https://.*pull/)([0-9]+).*$` by `[#$2]: $1$2`. - In the original, replace `(https://.*pull/)([0-9]+)` by `[#$2]`. - Copy a section of new contributors. - In the copy, replace `\* @([^ ]+) .*$` by `[@$1]: https://githhub.com/$1`. - In the original, replace `@([a-zA-Z0-9]+)` by `[@$1]`. Add (co)authors in [just-the-docs#578]: * [@AdityaTiwari2102] made their first contribution in [just-the-docs#477] * [@svrooij] made their first contribution in [just-the-docs#544] [@AdityaTiwari2102]: https://githhub.com/AdityaTiwari2102 [@svrooij]: https://githhub.com/svrooij To test: 1. Check that the CHANGELOG contents looks the same in the docs and in the repo (apart from the callouts) with clickable links to all PRs and contributors for v0.4.0.rc1. 2. Check that none of the lines with changes or new contributors have been removed.
* Make more CHANGELOG urls clickable Extension of PR #977 to v0.4.0.rc1: - Make callouts `loud`. - Replace a warning paragraph by a callout. - Use a reference link instead of an explicit url for @pmarsceill . - Add link reference definitions for PRs and new contributors in v0.4.0.rc1. - Replace inline urls by shortcut link references. The changes to replace URLs by link references would be too tedious and error-prone to do completely manually for v0.4.0.rc1. The following regexp replacements were applied to the sections of v0.4.0.rc1, using Atom: - Copy a section of changes. - In the copy, replace `.*(https://.*pull/)([0-9]+).*$` by `[#$2]: $1$2`. - In the original, replace `(https://.*pull/)([0-9]+)` by `[#$2]`. - Copy a section of new contributors. - In the copy, replace `\* @([^ ]+) .*$` by `[@$1]: https://githhub.com/$1`. - In the original, replace `@([a-zA-Z0-9]+)` by `[@$1]`. Add (co)authors in [#578]: * [@AdityaTiwari2102] made their first contribution in [#477] * [@svrooij] made their first contribution in [#544] [@AdityaTiwari2102]: https://githhub.com/AdityaTiwari2102 [@svrooij]: https://githhub.com/svrooij To test: 1. Check that the CHANGELOG contents looks the same in the docs and in the repo (apart from the callouts) with clickable links to all PRs and contributors for v0.4.0.rc1. 2. Check that none of the lines with changes or new contributors have been removed. * Revert to quiet callouts
This PR is for the improvement of breadcrumb present in the pages which will allow generating the proper and correct URL when a
parentpage with the same name is present in multiplegrand_parentpages. Currently, the appended URL path in the breadcrumb is of the last grandparent based on thenav_order.Example:
Based on the above tree structure, when a breadcrumb is generated for the children of
Parent-pageit will have the second level URL forGrand-parent-page-2in the breadcrumb even if the child is part of a different grandparent.A possible solution for this is to have a condition in place to check the grandparent of the page for the child as well.