From d1dc9ef72d80800728a0e07e47cbee94f66bf3d0 Mon Sep 17 00:00:00 2001
From: Ynda Jas <yndajas@gmail.com>
Date: Thu, 27 Jun 2024 16:01:37 +0100
Subject: [PATCH] Make applications table vertical on small screens

Since #2341, we've had support for making tables use a vertical layout
on smaller viewports, similar to how the summary list reflows on smaller
viewports (the end result is something of a hybrid of table and summary
list layouts)

We already use this on the users screen. This feels like something that
would be useful on the applications screen too

I've added an optional `visually_hidden` flag on table head cells here,
which when set to `true` will result in a class that removes padding
from the header cell. The current actions column on the applications
page has a visually hidden header, so this allows us to remove the
padding from visually hidden header cells while retaining the content

Per f0a8f53, we switch to regex-based matching for some tests. The
vertical layout implementation adds some hidden text in table cells that
would otherwise break these `assert_select`s
---
 app/assets/stylesheets/application.scss       | 23 ++++++++++++------
 app/assets/stylesheets/components/_table.scss |  4 ++++
 app/views/account/applications/index.html.erb |  6 +++--
 .../api_users/applications/index.html.erb     |  3 ++-
 app/views/components/_table.html.erb          |  5 ++--
 app/views/users/applications/index.html.erb   |  6 +++--
 .../account/applications_controller_test.rb   | 24 +++++++++----------
 .../api_users/applications_controller_test.rb |  6 ++---
 .../users/applications_controller_test.rb     |  8 +++----
 9 files changed, 52 insertions(+), 33 deletions(-)

diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss
index 635a7b10c2..d59a84e1bb 100644
--- a/app/assets/stylesheets/application.scss
+++ b/app/assets/stylesheets/application.scss
@@ -8,6 +8,8 @@ $govuk-page-width: 1140px;
 @import "components/contact-details";
 @import "components/table";
 
+$applications-table-actions-gap: 10px;
+
 // TODO: move into component
 .gem-c-success-alert,
 .gem-c-error-alert {
@@ -88,15 +90,22 @@ $govuk-page-width: 1140px;
   }
 }
 
-.govuk-table__actions {
-  display: flex;
-  flex-direction: column;
-  align-items: end;
-  gap: 10px;
-  
-  @media (min-width: 40.0625em) {
+.govuk-table__actions { 
+  @include govuk-media-query($until: $vertical-on-smallscreen-breakpoint) {
+    .govuk-button, .govuk-link {
+      display: block;
+
+      &:not(:last-child) {
+        margin-bottom: $applications-table-actions-gap;
+      }
+    }
+  }
+
+  @include govuk-media-query($from: $vertical-on-smallscreen-breakpoint) {
+    display: flex;
     flex-direction: row;
     align-items: center;
     justify-content: flex-end;
+    gap: $applications-table-actions-gap;
   }
 }
diff --git a/app/assets/stylesheets/components/_table.scss b/app/assets/stylesheets/components/_table.scss
index 9abdc1bd37..8fe9136a09 100644
--- a/app/assets/stylesheets/components/_table.scss
+++ b/app/assets/stylesheets/components/_table.scss
@@ -176,6 +176,10 @@ $table-row-even-background-colour: govuk-colour("light-grey", $legacy: "grey-4")
     padding-right: 1em;
     text-align: left;
     word-break: initial;
+
+    &--visually-hidden {
+      padding-right: 0;
+    }
   }
 
   @include govuk-media-query($from: $vertical-on-smallscreen-breakpoint) {
diff --git a/app/views/account/applications/index.html.erb b/app/views/account/applications/index.html.erb
index bc19421dad..75f90dd17d 100644
--- a/app/views/account/applications/index.html.erb
+++ b/app/views/account/applications/index.html.erb
@@ -30,7 +30,7 @@
     head: [
       { text: "Name" },
       { text: "Description" },
-      { text: content_tag(:span, "Actions", class: "govuk-visually-hidden") },
+      { text: content_tag(:span, "Actions", class: "govuk-visually-hidden"), visually_hidden: true },
     ],
     rows: @applications_with_signin.map do |application|
     [
@@ -43,6 +43,7 @@
       },
     ]
     end,
+    vertical_on_small_screen: true,
 } %>
 
 <div class="govuk-table--with-actions">
@@ -51,7 +52,7 @@
     head: [
       { text: "Name" },
       { text: "Description" },
-      { text: content_tag(:span, "Actions", class: "govuk-visually-hidden") }
+      { text: content_tag(:span, "Actions", class: "govuk-visually-hidden"), visually_hidden: true }
     ],
     rows: @applications_without_signin.map do |application|
     [
@@ -60,5 +61,6 @@
       { text: wrap_links_in_actions_markup([account_applications_grant_access_link(application)]) }
     ]
     end,
+    vertical_on_small_screen: true,
 } %>
 </div>
diff --git a/app/views/api_users/applications/index.html.erb b/app/views/api_users/applications/index.html.erb
index de4235dbfb..2294038452 100644
--- a/app/views/api_users/applications/index.html.erb
+++ b/app/views/api_users/applications/index.html.erb
@@ -35,7 +35,7 @@
     head: [
       { text: "Name" },
       { text: "Description" },
-      { text: content_tag(:span, "Actions", class: "govuk-visually-hidden") },
+      { text: content_tag(:span, "Actions", class: "govuk-visually-hidden"), visually_hidden: true },
     ],
     rows: @applications.map do |application|
     [
@@ -44,4 +44,5 @@
       { text: wrap_links_in_actions_markup([api_users_applications_permissions_link(application, @api_user)]) }
     ]
     end,
+    vertical_on_small_screen: true,
 } %>
diff --git a/app/views/components/_table.html.erb b/app/views/components/_table.html.erb
index 5ec626729c..712a62081b 100644
--- a/app/views/components/_table.html.erb
+++ b/app/views/components/_table.html.erb
@@ -30,7 +30,8 @@
             format: item[:format],
             href: item[:href],
             data_attributes: item[:data_attributes],
-            sort_direction: item[:sort_direction]
+            sort_direction: item[:sort_direction],
+            visually_hidden: item[:visually_hidden]
           } %>
         <% end %>
       <% end %>
@@ -47,7 +48,7 @@
               } %>
             <% elsif vertical_on_small_screen && head.any? %>
               <%= t.cell nil, { format: cell[:format] } do %>
-                <span class="app-c-table__duplicate-heading" aria-hidden="true">
+                <span class="app-c-table__duplicate-heading<%= head[cellindex][:visually_hidden] ? " app-c-table__duplicate-heading--visually-hidden" : "" %>" aria-hidden="true">
                   <%= head[cellindex][:text] %>
                 </span>
                 <%= cell[:text] %>
diff --git a/app/views/users/applications/index.html.erb b/app/views/users/applications/index.html.erb
index e390853c09..d05eeea50a 100644
--- a/app/views/users/applications/index.html.erb
+++ b/app/views/users/applications/index.html.erb
@@ -35,7 +35,7 @@
     head: [
       { text: "Name" },
       { text: "Description" },
-      { text: content_tag(:span, "Actions", class: "govuk-visually-hidden") },
+      { text: content_tag(:span, "Actions", class: "govuk-visually-hidden"), visually_hidden: true },
     ],
     rows: @applications_with_signin.map do |application|
     [
@@ -48,6 +48,7 @@
       },
     ]
     end,
+    vertical_on_small_screen: true,
 } %>
 
 <%= render "components/table", {
@@ -55,7 +56,7 @@
     head: [
       { text: "Name" },
       { text: "Description" },
-      { text: content_tag(:span, "Actions", class: "govuk-visually-hidden") }
+      { text: content_tag(:span, "Actions", class: "govuk-visually-hidden"), visually_hidden: true }
     ],
     rows: @applications_without_signin.map do |application|
     [
@@ -64,4 +65,5 @@
       { text: wrap_links_in_actions_markup([users_applications_grant_access_link(application, @user)]) }
     ]
     end,
+    vertical_on_small_screen: true,
 } %>
diff --git a/test/controllers/account/applications_controller_test.rb b/test/controllers/account/applications_controller_test.rb
index fc248a68cb..ede99bfc49 100644
--- a/test/controllers/account/applications_controller_test.rb
+++ b/test/controllers/account/applications_controller_test.rb
@@ -35,7 +35,7 @@ class Account::ApplicationsControllerTest < ActionController::TestCase
 
         get :index
 
-        assert_select "tr td", text: "app-name"
+        assert_select "tr td", text: /app-name/
         assert_select "form[action='#{account_application_signin_permission_path(application)}']"
       end
 
@@ -46,7 +46,7 @@ class Account::ApplicationsControllerTest < ActionController::TestCase
 
         get :index
 
-        assert_select "tr td", text: "app-name"
+        assert_select "tr td", text: /app-name/
         assert_select "a[href='#{delete_account_application_signin_permission_path(application)}']"
       end
 
@@ -57,7 +57,7 @@ class Account::ApplicationsControllerTest < ActionController::TestCase
 
         get :index
 
-        assert_select "tr td", text: "app-name"
+        assert_select "tr td", text: /app-name/
         assert_select "a[href='#{edit_account_application_permissions_path(application)}']"
       end
 
@@ -68,7 +68,7 @@ class Account::ApplicationsControllerTest < ActionController::TestCase
 
         get :index
 
-        assert_select "tr td", text: "app-name"
+        assert_select "tr td", text: /app-name/
         assert_select "a[href='#{edit_account_application_permissions_path(application)}']", count: 0
       end
 
@@ -78,7 +78,7 @@ class Account::ApplicationsControllerTest < ActionController::TestCase
 
         get :index
 
-        assert_select "tr td", text: "retired-app-name", count: 0
+        assert_select "tr td", text: /retired-app-name/, count: 0
       end
 
       should "not display an API-only application" do
@@ -87,7 +87,7 @@ class Account::ApplicationsControllerTest < ActionController::TestCase
 
         get :index
 
-        assert_select "tr td", text: "api-only-app-name", count: 0
+        assert_select "tr td", text: /api-only-app-name/, count: 0
       end
     end
 
@@ -102,7 +102,7 @@ class Account::ApplicationsControllerTest < ActionController::TestCase
 
         get :index
 
-        assert_select "tr td", text: "app-name"
+        assert_select "tr td", text: /app-name/
         assert_select "form[action='#{account_application_signin_permission_path(@application)}']", count: 0
       end
 
@@ -116,7 +116,7 @@ class Account::ApplicationsControllerTest < ActionController::TestCase
 
           get :index
 
-          assert_select "tr td", text: "app-name"
+          assert_select "tr td", text: /app-name/
           assert_select "a[href='#{delete_account_application_signin_permission_path(@application)}']"
         end
 
@@ -127,7 +127,7 @@ class Account::ApplicationsControllerTest < ActionController::TestCase
 
           get :index
 
-          assert_select "tr td", text: "app-name"
+          assert_select "tr td", text: /app-name/
           assert_select "a[href='#{edit_account_application_permissions_path(@application)}']"
         end
 
@@ -136,7 +136,7 @@ class Account::ApplicationsControllerTest < ActionController::TestCase
 
           get :index
 
-          assert_select "tr td", text: "app-name"
+          assert_select "tr td", text: /app-name/
           assert_select "a[href='#{edit_account_application_permissions_path(@application)}']", count: 0
         end
 
@@ -150,7 +150,7 @@ class Account::ApplicationsControllerTest < ActionController::TestCase
 
             get :index
 
-            assert_select "tr td", text: "app-name"
+            assert_select "tr td", text: /app-name/
             assert_select "a[href='#{delete_account_application_signin_permission_path(@application)}']", count: 0
           end
 
@@ -159,7 +159,7 @@ class Account::ApplicationsControllerTest < ActionController::TestCase
 
             get :index
 
-            assert_select "tr td", text: "app-name"
+            assert_select "tr td", text: /app-name/
             assert_select "a[href='#{account_application_permissions_path(@application)}']"
           end
         end
diff --git a/test/controllers/api_users/applications_controller_test.rb b/test/controllers/api_users/applications_controller_test.rb
index afbc808cde..9c1bcfecfd 100644
--- a/test/controllers/api_users/applications_controller_test.rb
+++ b/test/controllers/api_users/applications_controller_test.rb
@@ -37,7 +37,7 @@ class ApiUsers::ApplicationsControllerTest < ActionController::TestCase
       get :index, params: { api_user_id: api_user }
 
       assert_select "table:has( > caption[text()='Apps #{api_user.name} has access to'])" do
-        assert_select "tr td", text: "app-name"
+        assert_select "tr td", text: /app-name/
       end
     end
 
@@ -54,7 +54,7 @@ class ApiUsers::ApplicationsControllerTest < ActionController::TestCase
 
       get :index, params: { api_user_id: api_user }
 
-      assert_select "tr td", text: "revoked-app-name", count: 0
+      assert_select "tr td", text: /revoked-app-name/, count: 0
     end
 
     should "not display a retired application" do
@@ -70,7 +70,7 @@ class ApiUsers::ApplicationsControllerTest < ActionController::TestCase
 
       get :index, params: { api_user_id: api_user }
 
-      assert_select "tr td", text: "retired-app-name", count: 0
+      assert_select "tr td", text: /retired-app-name/, count: 0
     end
 
     should "display a flash message showing the permissions the user has" do
diff --git a/test/controllers/users/applications_controller_test.rb b/test/controllers/users/applications_controller_test.rb
index 4c65b1c91d..5f40e39c2d 100644
--- a/test/controllers/users/applications_controller_test.rb
+++ b/test/controllers/users/applications_controller_test.rb
@@ -77,7 +77,7 @@ class Users::ApplicationsControllerTest < ActionController::TestCase
       get :index, params: { user_id: user }
 
       assert_select "table:has( > caption[text()='Apps #{user.name} has access to'])" do
-        assert_select "tr td", text: "app-name"
+        assert_select "tr td", text: /app-name/
       end
     end
 
@@ -94,7 +94,7 @@ class Users::ApplicationsControllerTest < ActionController::TestCase
       get :index, params: { user_id: user }
 
       assert_select "table:has( > caption[text()='Apps #{user.name} does not have access to'])" do
-        assert_select "tr td", text: "app-name"
+        assert_select "tr td", text: /app-name/
       end
     end
 
@@ -232,7 +232,7 @@ class Users::ApplicationsControllerTest < ActionController::TestCase
 
       get :index, params: { user_id: user }
 
-      assert_select "tr td", text: "retired-app-name", count: 0
+      assert_select "tr td", text: /retired-app-name/, count: 0
     end
 
     should "not display an API-only application" do
@@ -247,7 +247,7 @@ class Users::ApplicationsControllerTest < ActionController::TestCase
 
       get :index, params: { user_id: user }
 
-      assert_select "tr td", text: "api-only-app-name", count: 0
+      assert_select "tr td", text: /api-only-app-name/, count: 0
     end
   end