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 missing <ul> in single page navigation #251

Merged
merged 1 commit into from
Jul 29, 2021
Merged

Conversation

36degrees
Copy link
Member

@36degrees 36degrees commented Jul 29, 2021

Since the changes made in 30471a9, there’s now no outer <ul> when multipage_nav is false – it jumps straight from the <nav> to the <li>.

Because multi_page_table_of_contents calls single_page_table_of_contents via render_page_tree, it's not as simple as adding the extra <ul> to the existing single_page_table_of_contents method, otherwise you end up with duplicate <ul> elements in some cases.

Instead, rename the existing single_page_table_of_contents to create a new function that can be called by both render_page_tree and a new single_page_table_of_contents method. The new single_page_table_of_contents can then add the wrapper without affecting the output of multi_page_table_of_contents.

Fixes #250

@36degrees
Copy link
Member Author

Raised as a draft as I'm not super happy with this as a fix, mainly because it adds yet another level of indirection. I feel like there's probably a refactor here which simplifies things, but I just can't see it.

@richardTowers
Copy link
Contributor

I've had a reasonable look, and I think this is a good approach. I agree there are some concerns with the way it's factored - as I see it:

  • It's impossible to tell from the module which methods are supposed to be public (single_page_table_of_contents and multi_page_table_of_contents) and which are implementation details (table_of_contents and render_page_tree). If someone accidentally calls table_of_contents directly they won't get the <ul>s.

  • The logic to "wrap stuff in <ul>s" is duplicated in single_page_table_of_contents and render_page_tree

I also can't immediately see a way to refactor this that doesn't just make it even more complicated (e.g. you could hide the table_of_contents / render_page_tree from the public interface of the module somehow, but I think you'd need to introduce a class or something).

Maybe it's just worth thinking about the name of table_of_contents. Maybe it could be list_items_from_headings or something less tempting to call externally?

Since the changes made in 30471a9, there’s now no outer <ul> when `multipage_nav` is false – it jumps straight from the `<nav>` to the `<li>`.

Because `multi_page_table_of_contents ` calls `single_page_table_of_contents` via `render_page_tree`, it's not as simple as adding the extra `<ul>` to the existing `single_page_table_of_contents` method, otherwise you end up with duplicate `<ul>` elements in some cases.

Instead, rename the existing `single_page_table_of_contents` to create a new function `list_items_from_headings `. The `list_items_from_headings` method can be called by both `render_page_tree` and a new `single_page_table_of_contents` method which maintains the existing API. This new `single_page_table_of_contents` method can then add the wrapper without affecting the output of `multi_page_table_of_contents`.
@36degrees
Copy link
Member Author

Maybe it's just worth thinking about the name of table_of_contents. Maybe it could be list_items_from_headings or something less tempting to call externally?

This is a good suggestion, thank you 👍🏻 I've changed it by amended the existing commit.

@36degrees 36degrees marked this pull request as ready for review July 29, 2021 12:29
@36degrees
Copy link
Member Author

Tested with the GDS Way repo and the navigation appears to be correctly formed:

<nav id="toc" class="js-toc-list toc__list" aria-labelledby="toc-heading">
  <ul>
    <li>
      <a href="#how-to-review-code"><span>How to review code</span></a>
      <ul>
        <li>
          <a href="#good-review-practice" class="toc-link--in-view"><span>Good review practice</span></a>
        </li>
        <li>
          <a href="#programming-style" class=""><span>Programming style</span></a>
        </li>
        <li>
          <a href="#code-libraries" class=""><span>Code libraries</span></a>
        </li>
        <li>
          <a href="#third-party-dependencies" class=""><span>Third-party dependencies</span></a>
        </li>
        <li>
          <a href="#testing" class=""><span>Testing</span></a>
        </li>
        <li>
          <a href="#deployment" class=""><span>Deployment</span></a>
        </li>
        <li>
          <a href="#documentation" class=""><span>Documentation</span></a>
        </li>
        <li>
          <a href="#further-reading"><span>Further reading</span></a>
        </li>
      </ul>
    </li>
  </ul>
</nav>

@36degrees 36degrees merged commit f563c3e into master Jul 29, 2021
@36degrees 36degrees deleted the fix-single-page-nav-ul branch July 29, 2021 13:18
@timja
Copy link
Contributor

timja commented Oct 25, 2021

This seems to prevent including partial markdown files?

image

@richardTowers
Copy link
Contributor

Hi @timja - could you raise a separate issue with more details of where you're seeing this error? If your code is open source we can have a quick look, otherwise you'll need to say a bit more about what you're doing that you think causes the error.

@timja
Copy link
Contributor

timja commented Oct 25, 2021

@richardTowers #277

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.

Sidebar navigation is missing outer <ul> if multipage_nav is disabled
3 participants