From ee69f46da3216687af2a1ebc829366bb4e29eb6d Mon Sep 17 00:00:00 2001
From: Paul Robert Lloyd <me+git@paulrobertlloyd.com>
Date: Tue, 24 May 2022 14:42:41 +0100
Subject: [PATCH] Use seperate components for primary and sub navigation

---
 app/components/navigation_component.html.erb  | 17 --------
 app/components/navigation_component.rb        | 13 -------
 .../primary_navigation_component.html.erb     | 17 ++++++++
 .../primary_navigation_component.rb           | 12 ++++++
 .../sub_navigation_component.html.erb         | 15 +++++++
 app/components/sub_navigation_component.rb    | 12 ++++++
 app/frontend/styles/_sub-navigation.scss      |  1 +
 app/views/layouts/application.html.erb        |  3 +-
 .../organisations/{logs.erb => logs.html.erb} | 12 +++---
 app/views/organisations/show.html.erb         | 13 +++----
 .../primary_navigation_component_spec.rb      | 18 ++++-----
 .../sub_navigation_component_spec.rb          | 39 +++++++++++++++++++
 12 files changed, 116 insertions(+), 56 deletions(-)
 delete mode 100644 app/components/navigation_component.html.erb
 delete mode 100644 app/components/navigation_component.rb
 create mode 100644 app/components/primary_navigation_component.html.erb
 create mode 100644 app/components/primary_navigation_component.rb
 create mode 100644 app/components/sub_navigation_component.html.erb
 create mode 100644 app/components/sub_navigation_component.rb
 rename app/views/organisations/{logs.erb => logs.html.erb} (77%)
 create mode 100644 spec/components/sub_navigation_component_spec.rb

diff --git a/app/components/navigation_component.html.erb b/app/components/navigation_component.html.erb
deleted file mode 100644
index 3d192ef2b..000000000
--- a/app/components/navigation_component.html.erb
+++ /dev/null
@@ -1,17 +0,0 @@
-<nav class="app-<%= level %>-navigation" aria-label="<%= level %>">
-  <div class="govuk-width-container">
-    <ul class="app-<%= level %>-navigation__list">
-      <% items.each do |item| %>
-        <% if item.current %>
-          <li class="app-<%= level %>-navigation__item app-<%= level %>-navigation__item--current">
-            <%= govuk_link_to item[:text], item[:href], class: "app-#{level}-navigation__link", aria: { current: "page" } %>
-          </li>
-        <% else %>
-          <li class="app-<%= level %>-navigation__item">
-            <%= govuk_link_to item[:text], item[:href], class: "app-#{level}-navigation__link" %>
-          </li>
-        <% end %>
-      <% end %>
-    </ul>
-  </div>
-</nav>
diff --git a/app/components/navigation_component.rb b/app/components/navigation_component.rb
deleted file mode 100644
index e386e429f..000000000
--- a/app/components/navigation_component.rb
+++ /dev/null
@@ -1,13 +0,0 @@
-class NavigationComponent < ViewComponent::Base
-  attr_reader :items, :level
-
-  def initialize(items:, level: "primary")
-    @items = items
-    @level = level
-    super
-  end
-
-  def highlighted_tab?(item, _path)
-    item[:current]
-  end
-end
diff --git a/app/components/primary_navigation_component.html.erb b/app/components/primary_navigation_component.html.erb
new file mode 100644
index 000000000..9cad3bc64
--- /dev/null
+++ b/app/components/primary_navigation_component.html.erb
@@ -0,0 +1,17 @@
+<nav class="app-primary-navigation" aria-label="primary">
+  <div class="govuk-width-container">
+    <ul class="app-primary-navigation__list">
+      <% items.each do |item| %>
+        <% if item.current %>
+          <li class="app-primary-navigation__item app-primary-navigation__item--current">
+            <%= govuk_link_to item[:text], item[:href], class: "app-primary-navigation__link", aria: { current: "page" } %>
+          </li>
+        <% else %>
+          <li class="app-primary-navigation__item">
+            <%= govuk_link_to item[:text], item[:href], class: "app-primary-navigation__link" %>
+          </li>
+        <% end %>
+      <% end %>
+    </ul>
+  </div>
+</nav>
diff --git a/app/components/primary_navigation_component.rb b/app/components/primary_navigation_component.rb
new file mode 100644
index 000000000..0389d30b3
--- /dev/null
+++ b/app/components/primary_navigation_component.rb
@@ -0,0 +1,12 @@
+class PrimaryNavigationComponent < ViewComponent::Base
+  attr_reader :items
+
+  def initialize(items:)
+    @items = items
+    super
+  end
+
+  def highlighted_item?(item, _path)
+    item[:current]
+  end
+end
diff --git a/app/components/sub_navigation_component.html.erb b/app/components/sub_navigation_component.html.erb
new file mode 100644
index 000000000..b9790d917
--- /dev/null
+++ b/app/components/sub_navigation_component.html.erb
@@ -0,0 +1,15 @@
+<nav class="app-sub-navigation" aria-label="secondary">
+  <ul class="app-sub-navigation__list">
+    <% items.each do |item| %>
+      <% if item.current %>
+        <li class="app-sub-navigation__item app-sub-navigation__item--current">
+          <%= govuk_link_to item[:text], item[:href], class: "app-sub-navigation__link", aria: { current: "page" } %>
+        </li>
+      <% else %>
+        <li class="app-sub-navigation__item">
+          <%= govuk_link_to item[:text], item[:href], class: "app-sub-navigation__link" %>
+        </li>
+      <% end %>
+    <% end %>
+  </ul>
+</nav>
diff --git a/app/components/sub_navigation_component.rb b/app/components/sub_navigation_component.rb
new file mode 100644
index 000000000..e4dd0654a
--- /dev/null
+++ b/app/components/sub_navigation_component.rb
@@ -0,0 +1,12 @@
+class SubNavigationComponent < ViewComponent::Base
+  attr_reader :items
+
+  def initialize(items:)
+    @items = items
+    super
+  end
+
+  def highlighted_item?(item, _path)
+    item[:current]
+  end
+end
diff --git a/app/frontend/styles/_sub-navigation.scss b/app/frontend/styles/_sub-navigation.scss
index c930256a3..7940e4814 100644
--- a/app/frontend/styles/_sub-navigation.scss
+++ b/app/frontend/styles/_sub-navigation.scss
@@ -61,6 +61,7 @@
   @include govuk-link-style-no-visited-state;
   @include govuk-link-style-no-underline;
   @include govuk-typography-weight-bold;
+  position: relative;
 
   // Extend the touch area of the link to the list
   &:after {
diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb
index d1a2c0dd6..7a3e85d89 100644
--- a/app/views/layouts/application.html.erb
+++ b/app/views/layouts/application.html.erb
@@ -65,9 +65,8 @@
     ) %>
 
     <% if !current_user.nil? %>
-      <%= render NavigationComponent.new(
+      <%= render PrimaryNavigationComponent.new(
         items: primary_items(request.path, current_user),
-        level: "primary",
       ) %>
     <% end %>
 
diff --git a/app/views/organisations/logs.erb b/app/views/organisations/logs.html.erb
similarity index 77%
rename from app/views/organisations/logs.erb
rename to app/views/organisations/logs.html.erb
index cb72cd15d..a12287530 100644
--- a/app/views/organisations/logs.erb
+++ b/app/views/organisations/logs.html.erb
@@ -4,14 +4,12 @@
   <span class="govuk-caption-l"><%= @organisation.name %></span>
   <%= content_for(:title) %>
 </h1>
-<div class="govuk-!-margin-bottom-4">
-  <%= render NavigationComponent.new(
-      items: secondary_items(request.path, @organisation.id),
-      level: "sub"
-    ) %>
-</div>
-<div class="app-filter-layout" data-controller="filter-layout">
 
+<%= render SubNavigationComponent.new(
+  items: secondary_items(request.path, @organisation.id),
+) %>
+
+<div class="app-filter-layout" data-controller="filter-layout">
   <%= render partial: "case_logs/log_filters" %>
   <% if @case_logs.present? %>
     <div class="app-filter-layout__content">
diff --git a/app/views/organisations/show.html.erb b/app/views/organisations/show.html.erb
index dae556d97..ecd36da73 100644
--- a/app/views/organisations/show.html.erb
+++ b/app/views/organisations/show.html.erb
@@ -4,16 +4,13 @@
   <%= "Details" %>
 <% end %>
 
-<div class="govuk-grid-row">
-
 <% if current_user.support? %>
-  <div class="govuk-!-margin-bottom-4">
-    <%= render NavigationComponent.new(
-        items: secondary_items(request.path, @organisation.id),
-        level: "sub",
-      ) %>
-  </div>
+  <%= render SubNavigationComponent.new(
+    items: secondary_items(request.path, @organisation.id),
+  ) %>
 <% end %>
+
+<div class="govuk-grid-row">
   <div class="govuk-grid-column-two-thirds-from-desktop">
     <%= govuk_summary_list do |summary_list| %>
       <% @organisation.display_attributes.each do |attr| %>
diff --git a/spec/components/primary_navigation_component_spec.rb b/spec/components/primary_navigation_component_spec.rb
index e37f8320b..0de1c58d1 100644
--- a/spec/components/primary_navigation_component_spec.rb
+++ b/spec/components/primary_navigation_component_spec.rb
@@ -1,6 +1,6 @@
 require "rails_helper"
 
-RSpec.describe NavigationComponent, type: :component do
+RSpec.describe PrimaryNavigationComponent, type: :component do
   let(:items) do
     [
       NavigationItemsHelper::NavigationItem.new("Organisations", "/organisations", true),
@@ -9,8 +9,8 @@ RSpec.describe NavigationComponent, type: :component do
     ]
   end
 
-  context "when the item is 'current' in nav tabs" do
-    it "then that tab appears as selected" do
+  context "when the item is 'current' in nav items" do
+    it "then that item appears as selected" do
       result = render_inline(described_class.new(items:))
 
       expect(result.css('.app-primary-navigation__link[aria-current="page"]').text).to include("Organisations")
@@ -18,17 +18,17 @@ RSpec.describe NavigationComponent, type: :component do
   end
 
   context "when the current page is sub-page" do
-    it "highlights the correct tab" do
+    it "highlights the correct item" do
       navigation_panel = described_class.new(items:)
 
-      expect(navigation_panel).to be_highlighted_tab(items[0], "/something-else")
-      expect(navigation_panel).not_to be_highlighted_tab(items[1], "/something-else")
-      expect(navigation_panel).not_to be_highlighted_tab(items[2], "/something-else")
+      expect(navigation_panel).to be_highlighted_item(items[0], "/something-else")
+      expect(navigation_panel).not_to be_highlighted_item(items[1], "/something-else")
+      expect(navigation_panel).not_to be_highlighted_item(items[2], "/something-else")
     end
   end
 
-  context "when rendering tabs" do
-    it "all of the nav tabs specified in the items hash are passed to it" do
+  context "when rendering items" do
+    it "all of the nav items specified in the items hash are passed to it" do
       result = render_inline(described_class.new(items:))
 
       expect(result.text).to include("Organisations")
diff --git a/spec/components/sub_navigation_component_spec.rb b/spec/components/sub_navigation_component_spec.rb
new file mode 100644
index 000000000..fcfa2d75d
--- /dev/null
+++ b/spec/components/sub_navigation_component_spec.rb
@@ -0,0 +1,39 @@
+require "rails_helper"
+
+RSpec.describe SubNavigationComponent, type: :component do
+  let(:items) do
+    [
+      NavigationItemsHelper::NavigationItem.new("Organisations", "/organisations", true),
+      NavigationItemsHelper::NavigationItem.new("Users", "/users", false),
+      NavigationItemsHelper::NavigationItem.new("Logs ", "/logs", false),
+    ]
+  end
+
+  context "when the item is 'current' in nav items" do
+    it "then that tab appears as selected" do
+      result = render_inline(described_class.new(items:))
+
+      expect(result.css('.app-sub-navigation__link[aria-current="page"]').text).to include("Organisations")
+    end
+  end
+
+  context "when the current page is sub-page" do
+    it "highlights the correct tab" do
+      navigation_panel = described_class.new(items:)
+
+      expect(navigation_panel).to be_highlighted_item(items[0], "/something-else")
+      expect(navigation_panel).not_to be_highlighted_item(items[1], "/something-else")
+      expect(navigation_panel).not_to be_highlighted_item(items[2], "/something-else")
+    end
+  end
+
+  context "when rendering items" do
+    it "all of the nav items specified in the items hash are passed to it" do
+      result = render_inline(described_class.new(items:))
+
+      expect(result.text).to include("Organisations")
+      expect(result.text).to include("Users")
+      expect(result.text).to include("Logs")
+    end
+  end
+end