From 4fd4a60fe603d48c62cda54a92f23049f51c0502 Mon Sep 17 00:00:00 2001
From: Oliver Byford <oliver.byford@digital.cabinet-office.gov.uk>
Date: Thu, 29 Jul 2021 10:00:29 +0100
Subject: [PATCH] Fix missing `<ul>` in single page navigation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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 `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`.
---
 .../table_of_contents/helpers.rb              | 24 ++++++++++++-------
 spec/table_of_contents/helpers_spec.rb        |  4 ++++
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/lib/govuk_tech_docs/table_of_contents/helpers.rb b/lib/govuk_tech_docs/table_of_contents/helpers.rb
index 34ca2662..d2cb3730 100644
--- a/lib/govuk_tech_docs/table_of_contents/helpers.rb
+++ b/lib/govuk_tech_docs/table_of_contents/helpers.rb
@@ -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)
@@ -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
@@ -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],
diff --git a/spec/table_of_contents/helpers_spec.rb b/spec/table_of_contents/helpers_spec.rb
index 436e64b2..dd2a63bc 100644
--- a/spec/table_of_contents/helpers_spec.rb
+++ b/spec/table_of_contents/helpers_spec.rb
@@ -18,6 +18,7 @@ class Subject
       }
 
       expected_single_page_table_of_contents = %{
+<ul>
   <li>
     <a href="#fruit"><span>Fruit</span></a>
   </li>
@@ -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)
@@ -45,6 +47,7 @@ class Subject
       }
 
       expected_single_page_table_of_contents = %{
+<ul>
   <li>
     <a href="#fruit"><span>Fruit</span></a>
     <ul>
@@ -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)