Skip to content

Commit

Permalink
Fix missing <ul> in single page navigation
Browse files Browse the repository at this point in the history
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`.
  • Loading branch information
36degrees committed Jul 29, 2021
1 parent 8375d93 commit 4fd4a60
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 8 deletions.
24 changes: 16 additions & 8 deletions lib/govuk_tech_docs/table_of_contents/helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,11 @@ module GovukTechDocs
module TableOfContents
module Helpers
def single_page_table_of_contents(html, url: "", max_level: nil)
headings = HeadingsBuilder.new(html, url).headings

if headings.none? { |heading| heading.size == 1 }
raise "No H1 tag found. You have to at least add one H1 heading to the page: " + url
end
output = "<ul>\n"
output += list_items_from_headings(html, url: url, max_level: max_level)
output += "</ul>\n"

tree = HeadingTreeBuilder.new(headings).tree
HeadingTreeRenderer.new(tree, max_level: max_level).html
output
end

def multi_page_table_of_contents(resources, current_page, config, current_page_html = nil)
Expand All @@ -28,6 +25,17 @@ def multi_page_table_of_contents(resources, current_page, config, current_page_h
render_page_tree(resources, current_page, config, current_page_html)
end

def list_items_from_headings(html, url: "", max_level: nil)
headings = HeadingsBuilder.new(html, url).headings

if headings.none? { |heading| heading.size == 1 }
raise "No H1 tag found. You have to at least add one H1 heading to the page: " + url
end

tree = HeadingTreeBuilder.new(headings).tree
HeadingTreeRenderer.new(tree, max_level: max_level).html
end

def render_page_tree(resources, current_page, config, current_page_html)
# Sort by weight frontmatter
resources = resources
Expand Down Expand Up @@ -69,7 +77,7 @@ def render_page_tree(resources, current_page, config, current_page_html)
output += "</li>\n"
else
output +=
single_page_table_of_contents(
list_items_from_headings(
content,
url: resource.url,
max_level: config[:tech_docs][:max_toc_heading_level],
Expand Down
4 changes: 4 additions & 0 deletions spec/table_of_contents/helpers_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ class Subject
}

expected_single_page_table_of_contents = %{
<ul>
<li>
<a href="#fruit"><span>Fruit</span></a>
</li>
Expand All @@ -29,6 +30,7 @@ class Subject
</li>
</ul>
</li>
</ul>
}

expect(subject.single_page_table_of_contents(html).strip).to eq(expected_single_page_table_of_contents.strip)
Expand All @@ -45,6 +47,7 @@ class Subject
}

expected_single_page_table_of_contents = %{
<ul>
<li>
<a href="#fruit"><span>Fruit</span></a>
<ul>
Expand All @@ -65,6 +68,7 @@ class Subject
<li>
<a href="#bread"><span>Bread</span></a>
</li>
</ul>
}

expect(subject.single_page_table_of_contents(html).strip).to eq(expected_single_page_table_of_contents.strip)
Expand Down

0 comments on commit 4fd4a60

Please sign in to comment.