Skip to content

Conversation

@AdityaTiwari2102
Copy link

This PR is for the improvement of breadcrumb present in the pages which will allow generating the proper and correct URL when a parent page with the same name is present in multiple grand_parent pages. Currently, the appended URL path in the breadcrumb is of the last grandparent based on the nav_order.

Example:

+-- ..
|
|-- Grand-parent-page-1
|   |-- Parent-page
|   |   |-- index.md  (parent page)
|   |   |-- child-1.md
|   |   |-- child-2.md
|   |-- (other parent pages with children)
|   |
|-- Grand-parent-page-2
|   |-- Parent-page
|   |   |-- index.md  (parent page)
|   |   |-- child-3.md
|   |   |-- child-4.md
|   |-- (other md files, pages with no children)
|
+-- ..

Based on the above tree structure, when a breadcrumb is generated for the children of Parent-page it will have the second level URL for Grand-parent-page-2 in 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.

@pdmosses
Copy link
Contributor

pdmosses commented Nov 9, 2020

@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 _config.yml for how to include the results of the regression tests in the navigation when building the theme documentation pages.

@pdmosses pdmosses self-requested a review November 9, 2020 13:50
Copy link
Contributor

@pdmosses pdmosses left a comment

Choose a reason for hiding this comment

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

@AdityaTiwari2102
Copy link
Author

@pdmosses - I can remove the regression test for disambiguation with this PR as well then, right?

@pdmosses
Copy link
Contributor

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

@AdityaTiwari2102
Copy link
Author

@pdmosses - Understood. Thanks for being patient.

Is it possible for you to merge the PR?

@pdmosses
Copy link
Contributor

@AdityaTiwari2102

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 _config.yml:

remote_theme: AdityaTiwari2102/just-the-docs@refactor/breadcrumb
plugins:
  - jekyll-feed
  - jekyll-remote-theme

and this in my Gemfile:

gem "jekyll-remote-theme"
gem "just-the-docs", github: "AdityaTiwari2102/just-the-docs", branch: "refactor/breadcrumb"

The bundle update went OK:

...
Using jekyll-remote-theme 0.4.2
Using jekyll-seo-tag 2.7.1
Using just-the-docs 0.3.3 from https://github.com/AdityaTiwari2102/just-the-docs.git (at refactor/breadcrumb@d81d215)
Bundle updated!

But when jekyll tried to build the site I got:

Remote Theme: "AdityaTiwari2102/just-the-docs@refactor/breadcrumb" is not a valid remote theme

I haven't seen that before. The only thing that seems unusual about your branch is that its name includes a /.

pdmosses added a commit to pdmosses/just-the-docs that referenced this pull request Nov 20, 2020
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.
pdmosses added a commit to pdmosses/just-the-docs that referenced this pull request Nov 20, 2020
- 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
pdmosses added a commit to pdmosses/just-the-docs that referenced this pull request Feb 15, 2021
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.
@pdmosses pdmosses mentioned this pull request Feb 15, 2021
@pdmosses pdmosses mentioned this pull request Mar 31, 2022
@pdmosses pdmosses linked an issue Jun 25, 2022 that may be closed by this pull request
@mattxwang mattxwang changed the base branch from main to v0.4.0 July 4, 2022 19:29
@mattxwang
Copy link
Member

Hi @AdityaTiwari2102, thanks for your contribution, and I'm sorry we're only getting to reviewing this now.

I've set the target to v0.4.0, our release branch. It seems like there's a merge conflict - could you (and/or @pdmosses) resolve this?

@pdmosses
Copy link
Contributor

pdmosses commented Jul 5, 2022

@AdityaTiwari2102 In release candidate v0.4.0, the Liquid code changed by this PR has been moved to

{%- if page.url == child.url or page.parent == child.title and page.grand_parent == child.parent -%}

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.

@AdityaTiwari2102
Copy link
Author

Please let me know if I got it wrong.

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 _layouts/default.html to resolve the merge conflicts.

@mattxwang mattxwang changed the title Breadcrumb improvement Improve breadcrumb behaviour in disambiguating multiple grand_parents Jul 20, 2022
@mattxwang
Copy link
Member

@pdmosses could I defer one last review to you on this? If not, I can do it before v0.4.0.

@pdmosses
Copy link
Contributor

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

@mattxwang
Copy link
Member

@mattxwang this PR changes 0 files, so nothing to review.

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

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.

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!

@mattxwang mattxwang closed this Jul 20, 2022
pdmosses added a commit to pdmosses/just-the-docs that referenced this pull request Sep 24, 2022
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.
mattxwang pushed a commit that referenced this pull request Sep 26, 2022
* 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
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.

3 participants