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

Fix external links for sites with no pages #1021

Merged

Conversation

pdmosses
Copy link
Contributor

Fix #1020

  1. Move the display of nav external links from _includes/nav.html to _layouts/default.html.
  2. Replace unless include.key by if site.nav_external_links.
  3. Wrap the body of if site.nav_external_links in <ul class="nav-list">…</ul>.

To test this PR:

  1. Add to _config.yml:
    defaults:
      - 
        scope: {path: ""} 
        values: {nav_exclude: true}
  2. Rebuild the site.
  3. Check that the nav panel displays only external links.

Fix just-the-docs#1020

- Move the display of nav external links from `_includes/nav.html` to `_layouts/default.html`.
- Replace ` unless include.key` by `if site.nav_external_links`.
- Wrap the body of `if site.nav_external_links` in `<ul class="nav-list">…</ul>`.

To test this PR:

1.  Add to `_config.yml`:
    ```yaml
    defaults:
      -
        scope: {path: ""}
        values: {nav_exclude: true}
    ```

2.  Check that the only link to appear in the nav panel is external.
@pdmosses pdmosses requested review from mattxwang and a team and removed request for mattxwang October 30, 2022 14:37
@pdmosses
Copy link
Contributor Author

I've added a regression test for this issue.

@mattxwang
Copy link
Member

Thanks for taking a crack at this @pdmosses. I have a bit of limited time right now (I'll try to review / merge the other PRs later today/tomorrow), but just to check my understanding: the problem in #1020 is that pages_top_size in default.html is 0 when there's only one index page, so the nav isn't even being rendered?

I wonder if it's bad (for any reason) for us to have two different ul items for the nav. I'll have to think about it for a bit.

@pdmosses
Copy link
Contributor Author

... just to check my understanding: the problem in #1020 is that pages_top_size in default.html is 0 when there's only one index page, so the nav isn't even being rendered?

It's 0 when all ordinary pages are excluded from the nav panel by nav_exclude: true. The index page is not automatically excluded (unless it has no title).

Anyway, moving the external links code to the default layout should increase the modular flexibility of the latter.

I wonder if it's bad (for any reason) for us to have two different ul items for the nav. I'll have to think about it for a bit.

I didn't notice any difference in the appearance. But if we wanted, I think it would be easy enough to strip the uls there, and enclose the resulting sequence of lis in a single ul.

Copy link
Member

@mattxwang mattxwang left a comment

Choose a reason for hiding this comment

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

Gotcha. Have done a more thorough look through, and I think I better understand this change; in particular, I feel like the previous approach "overloaded" the include key (using its existence to "check" if we were in the non-collection nav, when ideally, external links are considered separate from page nav elements).

Later down the line (perhaps motivated by #959), I think we should eventually move this to its own sandboxed file/include. Haven't gotten around to implementing it yet though, so let's not put the cart before the horse. LGTM!

(and, thanks for pointing the original user to your test branch; seems like it's resolved the original issue, which is great!)

@mattxwang mattxwang merged commit 29b5d14 into just-the-docs:main Nov 5, 2022
@pdmosses pdmosses deleted the fix-external-links-without-pages branch November 5, 2022 10:43
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.

External Nav Links Not Showing Without Pages
2 participants