From fa48f49245367d73a2da62e7c54b27d95bb1b5f0 Mon Sep 17 00:00:00 2001 From: Ynda Jas Date: Thu, 27 Jun 2024 11:30:03 +0100 Subject: [PATCH] Fix row alignment in applications tables In 901963c, styles were added to right align the actions in the final column of the applications tables. This was described as adopting a Whitehall-style pattern, and it does follow the pattern of wrapping the applications tables in a `div` with the class `govuk-table--with-actions` and applying alignment styles to the last column in the wrapped table, however the actual applied styles are quite different. In Whitehall, it's a simple `text-align: right`, whereas in Signon flex-based styling was introduced. The commit message explained that this was done so that we could have a gap between elements, since in Signon we have buttons/button links in addition to simple text links However, since this change, we've had issues with the height of the cells in the actions column. This has led to some broken/misaligned row borders under certain conditions (variation in whether cells need to wrap their text content) Using flex on the actual table cell appears to be the issue here: we lose the consistent height that table cells provide for free. In this commit, we therefore pivot to wrapping the contents of actions cells in a classed `div` on which the styles are now applied, which retains the custom alignment and gap for the actions but restores the default table styling on the cell itself. This fixes the height/border/row alignment issues The `flex-direction` is also switched to `column` at smaller viewport sizes so that we don't end up with very narrow links/buttons --- app/assets/stylesheets/application.scss | 14 +++++++++----- app/helpers/application_table_helper.rb | 4 ++++ app/views/account/applications/index.html.erb | 10 ++++++---- app/views/api_users/applications/index.html.erb | 4 +--- app/views/users/applications/index.html.erb | 12 ++++++------ test/helpers/application_table_helper_test.rb | 16 ++++++++++++++++ 6 files changed, 42 insertions(+), 18 deletions(-) diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index fdbf22188..635a7b10c 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -88,11 +88,15 @@ $govuk-page-width: 1140px; } } -.govuk-table--with-actions { - .govuk-table__cell:last-child { - display: flex; - justify-content: flex-end; +.govuk-table__actions { + display: flex; + flex-direction: column; + align-items: end; + gap: 10px; + + @media (min-width: 40.0625em) { + flex-direction: row; align-items: center; - gap: 10px; + justify-content: flex-end; } } diff --git a/app/helpers/application_table_helper.rb b/app/helpers/application_table_helper.rb index 73df241e4..4bc774172 100644 --- a/app/helpers/application_table_helper.rb +++ b/app/helpers/application_table_helper.rb @@ -1,6 +1,10 @@ module ApplicationTableHelper include Pundit::Authorization + def wrap_links_in_actions_markup(links) + "
#{links.join}
".html_safe + end + def account_applications_grant_access_link(application) if policy([:account, Doorkeeper::Application]).grant_signin_permission? grant_access_link(application) diff --git a/app/views/account/applications/index.html.erb b/app/views/account/applications/index.html.erb index 46e275894..bc19421da 100644 --- a/app/views/account/applications/index.html.erb +++ b/app/views/account/applications/index.html.erb @@ -25,7 +25,6 @@ <% end %> <% end %> -
<%= render "components/table", { caption: "Apps you have access to", head: [ @@ -37,11 +36,14 @@ [ { text: application.name }, { text: application.description }, - { text: safe_join([account_applications_permissions_link(application), account_applications_remove_access_link(application)]) }, + { text: wrap_links_in_actions_markup([ + account_applications_permissions_link(application), + account_applications_remove_access_link(application) + ]) + }, ] end, } %> -
<%= render "components/table", { @@ -55,7 +57,7 @@ [ { text: application.name }, { text: application.description }, - { text: account_applications_grant_access_link(application) } + { text: wrap_links_in_actions_markup([account_applications_grant_access_link(application)]) } ] end, } %> diff --git a/app/views/api_users/applications/index.html.erb b/app/views/api_users/applications/index.html.erb index 0b7e9e048..de4235dbf 100644 --- a/app/views/api_users/applications/index.html.erb +++ b/app/views/api_users/applications/index.html.erb @@ -30,7 +30,6 @@ <% end %> <% end %> -
<%= render "components/table", { caption: "Apps #{@api_user.name} has access to", head: [ @@ -42,8 +41,7 @@ [ { text: application.name }, { text: application.description }, - { text: api_users_applications_permissions_link(application, @api_user) } + { text: wrap_links_in_actions_markup([api_users_applications_permissions_link(application, @api_user)]) } ] end, } %> -
diff --git a/app/views/users/applications/index.html.erb b/app/views/users/applications/index.html.erb index 854d7431e..e390853c0 100644 --- a/app/views/users/applications/index.html.erb +++ b/app/views/users/applications/index.html.erb @@ -30,7 +30,6 @@ <% end %> <% end %> -
<%= render "components/table", { caption: "Apps #{@user.name} has access to", head: [ @@ -42,13 +41,15 @@ [ { text: application.name }, { text: application.description }, - { text: safe_join([users_applications_permissions_link(application, @user), users_applications_remove_access_link(application, @user)]) }, + { text: wrap_links_in_actions_markup([ + users_applications_permissions_link(application, @user), + users_applications_remove_access_link(application, @user) + ]) + }, ] end, } %> -
-
<%= render "components/table", { caption: "Apps #{@user.name} does not have access to", head: [ @@ -60,8 +61,7 @@ [ { text: application.name }, { text: application.description }, - { text: users_applications_grant_access_link(application, @user) } + { text: wrap_links_in_actions_markup([users_applications_grant_access_link(application, @user)]) } ] end, } %> -
diff --git a/test/helpers/application_table_helper_test.rb b/test/helpers/application_table_helper_test.rb index 787738933..9e0595226 100644 --- a/test/helpers/application_table_helper_test.rb +++ b/test/helpers/application_table_helper_test.rb @@ -3,6 +3,22 @@ class ApplicationTableHelperTest < ActionView::TestCase include PunditHelpers + context "#wrap_links_in_actions_markup" do + should "wrap an array of links in a container with a class to apply actions styles" do + links = [ + "Destination one", + "Destination two", + ] + + expected_output = "
+ Destination one + Destination two +
".gsub(/>\s+<") + + assert_equal expected_output, wrap_links_in_actions_markup(links) + end + end + context "#account_applications_grant_access_link" do setup do @user = build(:user)