Merge pull request #240 from alphagov/accessibility-menu-structure

Update menu html structure so it's one single hierarchical list
alphagov · Jul 16, 2021 · 38d26fa · 38d26fa
2 parents fe79973 + 75edaa5
commit 38d26fa
Show file tree

Hide file tree

Showing 5 changed files with 36 additions and 40 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,6 +3,7 @@
 ## Unreleased
 
 - [Bump redcarpet to 3.5.1 to fix CVE-2020-26298](https://github.com/alphagov/tech-docs-gem/pull/226)
+- [#240: Update menu html structure so it's one single hierarchical list](https://github.com/alphagov/tech-docs-gem/pull/240)
 - [244: Don't change the focus of the page on initial load](https://github.com/alphagov/tech-docs-gem/pull/244)
 
 ## 2.3.0

diff --git a/lib/govuk_tech_docs/table_of_contents/heading_tree_renderer.rb b/lib/govuk_tech_docs/table_of_contents/heading_tree_renderer.rb
@@ -24,7 +24,7 @@ def render_tree(tree, indentation: DEFAULT_INDENTATION, level: nil)
         end
 
         if tree.children.any? && level < @max_level
-          output += indentation + "<ul>\n"
+          output += indentation + "<ul>\n" unless level.zero?
 
           tree.children.each do |child|
             output += indentation + INDENTATION_INCREMENT + "<li>\n"
@@ -36,7 +36,7 @@ def render_tree(tree, indentation: DEFAULT_INDENTATION, level: nil)
             output += indentation + INDENTATION_INCREMENT + "</li>\n"
           end
 
-          output += indentation + "</ul>\n"
+          output += indentation + "</ul>\n" unless level.zero?
         end
 
         output

diff --git a/lib/govuk_tech_docs/table_of_contents/helpers.rb b/lib/govuk_tech_docs/table_of_contents/helpers.rb
@@ -32,7 +32,8 @@ def render_page_tree(resources, current_page, config, current_page_html)
         # Sort by weight frontmatter
         resources = resources
         .sort_by { |r| [r.data.weight ? 0 : 1, r.data.weight || 0] }
-        output = "";
+
+        output = "<ul>\n"
         resources.each do |resource|
           # Skip from page tree if hide_in_navigation:true frontmatter
           next if resource.data.hide_in_navigation
@@ -63,9 +64,9 @@ def render_page_tree(resources, current_page, config, current_page_html)
             end
 
           if resource.children.any? && resource.url != home_url
-            output += %{<ul><li><a href="#{resource.url}"><span>#{resource.data.title}</span></a>\n}
+            output += %{<li><a href="#{resource.url}"><span>#{resource.data.title}</span></a>\n}
             output += render_page_tree(resource.children, current_page, config, current_page_html)
-            output += "</li></ul>"
+            output += "</li>\n"
           else
             output +=
               single_page_table_of_contents(
@@ -75,6 +76,8 @@ def render_page_tree(resources, current_page, config, current_page_html)
               )
           end
         end
+        output += "</ul>\n"
+
         output
       end
     end

diff --git a/spec/table_of_contents/heading_tree_renderer_spec.rb b/spec/table_of_contents/heading_tree_renderer_spec.rb
@@ -22,33 +22,29 @@
     }
 
     let(:expected_html_with_all_headings) {
-      <<~EOF
-      <ul>
-        <li>
-          <a href="#apples"><span>Apples</span></a>
-          <ul>
-            <li>
-              <a href="#apple-recipes"><span>Apple recipes</span></a>
-            </li>
-          </ul>
-        </li>
-        <li>
-          <a href="#oranges"><span>Oranges</span></a>
-        </li>
-      </ul>
+      <<-EOF
+  <li>
+    <a href="#apples"><span>Apples</span></a>
+    <ul>
+      <li>
+        <a href="#apple-recipes"><span>Apple recipes</span></a>
+      </li>
+    </ul>
+  </li>
+  <li>
+    <a href="#oranges"><span>Oranges</span></a>
+  </li>
       EOF
     }
 
     let(:expected_html_with_max_heading_of_one) {
-      <<~EOF
-      <ul>
-        <li>
-          <a href="#apples"><span>Apples</span></a>
-        </li>
-        <li>
-          <a href="#oranges"><span>Oranges</span></a>
-        </li>
-      </ul>
+      <<-EOF
+  <li>
+    <a href="#apples"><span>Apples</span></a>
+  </li>
+  <li>
+    <a href="#oranges"><span>Oranges</span></a>
+  </li>
       EOF
     }
 

diff --git a/spec/table_of_contents/helpers_spec.rb b/spec/table_of_contents/helpers_spec.rb
@@ -18,7 +18,6 @@ class Subject
       }
 
       expected_single_page_table_of_contents = %{
-<ul>
   <li>
     <a href="#fruit"><span>Fruit</span></a>
   </li>
@@ -30,7 +29,6 @@ class Subject
       </li>
     </ul>
   </li>
-</ul>
       }
 
       expect(subject.single_page_table_of_contents(html).strip).to eq(expected_single_page_table_of_contents.strip)
@@ -47,7 +45,6 @@ class Subject
       }
 
       expected_single_page_table_of_contents = %{
-<ul>
   <li>
     <a href="#fruit"><span>Fruit</span></a>
     <ul>
@@ -68,7 +65,6 @@ 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)
@@ -148,7 +144,8 @@ def add_children(children)
       }
 
       expected_multi_page_table_of_contents = %{
-<ul><li><a href="/index.html"><span>Index</span></a>
+<ul>
+<li><a href="/index.html"><span>Index</span></a>
 <ul>
   <li>
     <a href="/a.html"><span>Heading one</span></a>
@@ -158,8 +155,6 @@ def add_children(children)
       </li>
     </ul>
   </li>
-</ul>
-<ul>
   <li>
     <a href="/b.html"><span>Heading one</span></a>
     <ul>
@@ -169,7 +164,8 @@ def add_children(children)
     </ul>
   </li>
 </ul>
-</li></ul>
+</li>
+</ul>
       }
 
       expect(subject.multi_page_table_of_contents(resources, current_page, config, current_page_html).strip).to eq(expected_multi_page_table_of_contents.strip)
@@ -197,7 +193,8 @@ def add_children(children)
       }
 
       expected_multi_page_table_of_contents = %{
-<ul><li><a href="/prefix/index.html"><span>Index</span></a>
+<ul>
+<li><a href="/prefix/index.html"><span>Index</span></a>
 <ul>
   <li>
     <a href="/prefix/a.html"><span>Heading one</span></a>
@@ -207,8 +204,6 @@ def add_children(children)
       </li>
     </ul>
   </li>
-</ul>
-<ul>
   <li>
     <a href="/prefix/b.html"><span>Heading one</span></a>
     <ul>
@@ -218,7 +213,8 @@ def add_children(children)
     </ul>
   </li>
 </ul>
-</li></ul>
+</li>
+</ul>
       }
 
       expect(subject.multi_page_table_of_contents(resources, current_page, config, current_page_html).strip).to eq(expected_multi_page_table_of_contents.strip)