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

Sidebar navigation is missing outer <ul> if multipage_nav is disabled #250

Closed
36degrees opened this issue Jul 28, 2021 · 1 comment · Fixed by #251
Closed

Sidebar navigation is missing outer <ul> if multipage_nav is disabled #250

36degrees opened this issue Jul 28, 2021 · 1 comment · Fixed by #251
Assignees

Comments

@36degrees
Copy link
Member

36degrees commented Jul 28, 2021

There’s no outer <ul> when multipage_nav is false – it jumps straight from the <nav> to the <li>.

<nav id="toc" class="js-toc-list toc__list" aria-labelledby="toc-heading" data-module="collapsible-navigation">
                        <li>
    <a href="#the-title" class="toc-link--in-view"><span>The title</span></a>
    <ul>
      <li>
        <a href="#a-subheader"><span>A subheader</span></a>
        <ul>
          <li>
            <a href="#a-h3-subheader"><span>A h3 subheader</span></a>
          </li>
        </ul>
      </li>
      <li>
        <a href="#another-subheader"><span>Another subheader</span></a>
      </li>
    </ul>
  </li>
</nav>

This causes rendering issues, as seen in alphagov/gds-way#632.

Before

current (1)

After

new

@36degrees
Copy link
Member Author

The issue is visible in the spec:

expected_single_page_table_of_contents = %{
<li>
<a href="#fruit"><span>Fruit</span></a>
</li>
<li>
<a href="#apples"><span>Apples</span></a>
<ul>
<li>
<a href="#apple-recipes"><span>Apple recipes</span></a>
</li>
</ul>
</li>
}

expected_multi_page_table_of_contents = %{
<ul>
<li><a href="/index.html"><span>Index</span></a>
<ul>
<li>
<a href="/a.html"><span>Heading one</span></a>
<ul>
<li>
<a href="/a.html#heading-two"><span>Heading two</span></a>
</li>
</ul>
</li>
<li>
<a href="/b.html"><span>Heading one</span></a>
<ul>
<li>
<a href="/b.html#heading-two"><span>Heading two</span></a>
</li>
</ul>
</li>
</ul>
</li>
</ul>
}

The render_page_tree method called by multi_page_table_of_contents manually adds the outer <ul> so possibly the single_page_table_of_contents method just needs to do the same…?

We should probably add an example of a non multipage nav to the 'example' docs, although at the minute this is configured at the site level rather than per page.

@36degrees 36degrees self-assigned this Jul 28, 2021
36degrees added a commit that referenced this issue Jul 29, 2021
This should (hopefully) make it easier to catch issues like #250 when making changes to the navigation code.
36degrees added a commit that referenced this issue Jul 29, 2021
This should (hopefully) make it easier to catch issues like #250 when making changes to the navigation code.
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 a pull request may close this issue.

1 participant