search

alphagov / tech-docs-gem

Gem to distribute the tech docs project
https://tdt-documentation.london.cloudapps.digital/
MIT License
15 stars 38 forks source link

Fix missing `<ul>` in single page navigation #251

Closed 36degrees closed 3 years ago

36degrees commented 3 years ago

Since the changes made in 30471a97, 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 commented 3 years ago

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 commented 3 years ago

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:

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?

36degrees commented 3 years ago

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 commented 3 years ago

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>
timja commented 3 years ago

This seems to prevent including partial markdown files?

image

richardTowers commented 3 years ago

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 commented 3 years ago

@richardTowers https://github.com/alphagov/tech-docs-gem/issues/277