From 5fce1c2f50d34487ed48cb6c83834e8c444bc8f7 Mon Sep 17 00:00:00 2001 From: Ynda Jas Date: Tue, 1 Oct 2024 17:36:55 +0100 Subject: [PATCH] Update applications table layout At present, on viewports of between 940px and 1199px, the actions links/buttons are various shades of messy, owing to the fact that they share a single column, use `flex` alignment, and are of different widths This updates the layout so that the actions links/buttons are nicely aligned and consistently positioned at all viewport sizes and regardless of which of the four links/buttons appear in the given tables - The visible text is simplified, favouring a combination of a visible column heading of "Access" or "Permissions" combined with single-word actions. The text read by a screen reader in each column should remain the same: the part no longer visible in individual links/buttons is simply moved to the visually-hidden `span` - The access column is moved before the permissions column to make the column order more in sync between the two tables - The `description` column width is increased, retaining a good whitespace distribution across the table and potentially reducing the line count for apps with longer descriptions --- app/assets/stylesheets/application.scss | 13 +++++++ app/helpers/application_table_helper.rb | 10 +++--- app/views/account/applications/index.html.erb | 18 +++++----- app/views/users/applications/index.html.erb | 18 +++++----- test/helpers/application_table_helper_test.rb | 34 +++++++++---------- 5 files changed, 51 insertions(+), 42 deletions(-) diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index e72aba116..0644b15f5 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -121,3 +121,16 @@ $show-button-space: calc($show-button-width + $show-button-left-margin); padding-right: $show-button-space; } } + +.applications-table__remove_access_link { + width: auto; + min-width: 90px; +} + +.applications-table__permissions-action-links a { + display: inline-block; + + &:not(:last-child) { + margin-right: 1rem; + } +} diff --git a/app/helpers/application_table_helper.rb b/app/helpers/application_table_helper.rb index 88b03638d..396d7e79f 100644 --- a/app/helpers/application_table_helper.rb +++ b/app/helpers/application_table_helper.rb @@ -73,7 +73,7 @@ def grant_access_link(application, user = nil) path, class: "govuk-button govuk-!-margin-0", data: { module: "govuk-button" }, - ) { button_or_link_content("Grant access", "to", application.name) } + ) { button_or_link_content("Grant", "access to", application.name) } end def remove_access_link(application, user = nil) @@ -85,9 +85,9 @@ def remove_access_link(application, user = nil) link_to( path, - class: "govuk-button govuk-button--warning govuk-!-margin-0", + class: "govuk-button govuk-button--warning govuk-!-margin-0 applications-table__remove_access_link", data: { module: "govuk-button" }, - ) { button_or_link_content("Remove access", "to", application.name) } + ) { button_or_link_content("Remove", "access to", application.name) } end def view_permissions_link(application, user = nil) @@ -97,7 +97,7 @@ def view_permissions_link(application, user = nil) account_application_permissions_path(application) end - link_to(path, class: "govuk-link") { button_or_link_content("View permissions", "for", application.name) } + link_to(path, class: "govuk-link") { button_or_link_content("View", "permissions for", application.name) } end def update_permissions_link(application, user = nil) @@ -115,7 +115,7 @@ def update_permissions_link(application, user = nil) edit_user_application_permissions_path(user, application) end - link_to(path, class: "govuk-link") { button_or_link_content("Update permissions", "for", application.name) } + link_to(path, class: "govuk-link") { button_or_link_content("Update", "permissions for", application.name) } end def button_or_link_content(visible_text, visually_hidden_join_word, application_name) diff --git a/app/views/account/applications/index.html.erb b/app/views/account/applications/index.html.erb index 334572e10..5f0bee1ea 100644 --- a/app/views/account/applications/index.html.erb +++ b/app/views/account/applications/index.html.erb @@ -29,18 +29,16 @@ caption: "Apps you have access to", head: [ { text: "Name", classes: "govuk-!-width-one-quarter" }, - { text: "Description", classes: "govuk-!-width-one-third" }, - { text: content_tag(:span, "Actions", class: "govuk-visually-hidden"), visually_hidden: true }, + { text: "Description", classes: "govuk-!-width-one-half" }, + { text: "Access" }, + { text: "Permissions" } ], rows: @applications_with_signin.map do |application| [ { text: application.name }, { text: application.description }, - { text: wrap_links_in_actions_markup([ - account_applications_permissions_links(application), - account_applications_remove_access_link(application) - ]) - }, + { text: account_applications_remove_access_link(application) }, + { text: account_applications_permissions_links(application), classes: "applications-table__permissions-action-links" } ] end, vertical_on_small_screen: true, @@ -51,14 +49,14 @@ caption: "Apps you don't have access to", head: [ { text: "Name", classes: "govuk-!-width-one-quarter" }, - { text: "Description", classes: "govuk-!-width-one-third" }, - { text: content_tag(:span, "Actions", class: "govuk-visually-hidden"), visually_hidden: true } + { text: "Description", classes: "govuk-!-width-one-half" }, + { text: "Access" } ], rows: @applications_without_signin.map do |application| [ { text: application.name }, { text: application.description }, - { text: wrap_links_in_actions_markup([account_applications_grant_access_link(application)]) } + { text: account_applications_grant_access_link(application) } ] end, vertical_on_small_screen: true, diff --git a/app/views/users/applications/index.html.erb b/app/views/users/applications/index.html.erb index d230a79c0..3ddf9d81c 100644 --- a/app/views/users/applications/index.html.erb +++ b/app/views/users/applications/index.html.erb @@ -34,18 +34,16 @@ caption: "Apps #{@user.name} has access to", head: [ { text: "Name", classes: "govuk-!-width-one-quarter" }, - { text: "Description", classes: "govuk-!-width-one-third" }, - { text: content_tag(:span, "Actions", class: "govuk-visually-hidden"), visually_hidden: true }, + { text: "Description", classes: "govuk-!-width-one-half" }, + { text: "Access" }, + { text: "Permissions" } ], rows: @applications_with_signin.map do |application| [ { text: application.name }, { text: application.description }, - { text: wrap_links_in_actions_markup([ - users_applications_permissions_links(application, @user), - users_applications_remove_access_link(application, @user) - ]) - }, + { text: users_applications_remove_access_link(application, @user) }, + { text: users_applications_permissions_links(application, @user), classes: "applications-table__permissions-action-links" } ] end, vertical_on_small_screen: true, @@ -55,14 +53,14 @@ caption: "Apps #{@user.name} does not have access to", head: [ { text: "Name", classes: "govuk-!-width-one-quarter" }, - { text: "Description", classes: "govuk-!-width-one-third" }, - { text: content_tag(:span, "Actions", class: "govuk-visually-hidden"), visually_hidden: true } + { text: "Description", classes: "govuk-!-width-one-half" }, + { text: "Access" } ], rows: @applications_without_signin.map do |application| [ { text: application.name }, { text: application.description }, - { text: wrap_links_in_actions_markup([users_applications_grant_access_link(application, @user)]) } + { text: users_applications_grant_access_link(application, @user) } ] end, vertical_on_small_screen: true, diff --git a/test/helpers/application_table_helper_test.rb b/test/helpers/application_table_helper_test.rb index 78261dc7e..6a341205a 100644 --- a/test/helpers/application_table_helper_test.rb +++ b/test/helpers/application_table_helper_test.rb @@ -28,7 +28,7 @@ class ApplicationTableHelperTest < ActionView::TestCase should "generate a grant access button when the current user can grant the signin permission" do stub_policy @user, [:account, Doorkeeper::Application], grant_signin_permission?: true - assert_includes account_applications_grant_access_link(@application), "Grant access" + assert_includes account_applications_grant_access_link(@application), "Grant" end should "return an empty string when the current user cannot grant the signin permission" do @@ -54,7 +54,7 @@ class ApplicationTableHelperTest < ActionView::TestCase grant_signin_permission?: true, ) - assert_includes users_applications_grant_access_link(@application, @grantee), "Grant access" + assert_includes users_applications_grant_access_link(@application, @grantee), "Grant" end should "return an empty string when the current user cannot grant the signin permission" do @@ -78,7 +78,7 @@ class ApplicationTableHelperTest < ActionView::TestCase should "generate a remove access link when the current user can remove the signin permission" do stub_policy @user, [:account, @application], remove_signin_permission?: true - assert_includes account_applications_remove_access_link(@application), "Remove access" + assert_includes account_applications_remove_access_link(@application), "Remove" end should "return an empty string when the current user cannot remove the signin permission" do @@ -104,7 +104,7 @@ class ApplicationTableHelperTest < ActionView::TestCase remove_signin_permission?: true, ) - assert_includes users_applications_remove_access_link(@application, @grantee), "Remove access" + assert_includes users_applications_remove_access_link(@application, @grantee), "Remove" end should "return an empty string when the current user cannot remove the signin permission" do @@ -130,8 +130,8 @@ class ApplicationTableHelperTest < ActionView::TestCase result = account_applications_permissions_links(@application) - assert_includes result, "View permissions" - assert_includes result, "Update permissions" + assert_includes result, "View" + assert_includes result, "Update" end should "only generate a view link when the user can only view permissions" do @@ -139,8 +139,8 @@ class ApplicationTableHelperTest < ActionView::TestCase result = account_applications_permissions_links(@application) - assert_includes result, "View permissions" - assert_not_includes result, "Update permissions" + assert_includes result, "View" + assert_not_includes result, "Update" end should "only generate an update link when the user can only edit permissions" do @@ -148,8 +148,8 @@ class ApplicationTableHelperTest < ActionView::TestCase result = account_applications_permissions_links(@application) - assert_not_includes result, "View permissions" - assert_includes result, "Update permissions" + assert_not_includes result, "View" + assert_includes result, "Update" end should "return an empty string when the user can do neither" do @@ -178,8 +178,8 @@ class ApplicationTableHelperTest < ActionView::TestCase result = users_applications_permissions_links(@application, @grantee) - assert_includes result, "View permissions" - assert_includes result, "Update permissions" + assert_includes result, "View" + assert_includes result, "Update" end should "only generate a view link when the user can only view permissions" do @@ -193,8 +193,8 @@ class ApplicationTableHelperTest < ActionView::TestCase result = users_applications_permissions_links(@application, @grantee) - assert_includes result, "View permissions" - assert_not_includes result, "Update permissions" + assert_includes result, "View" + assert_not_includes result, "Update" end should "only generate an edit link when the user can only edit permissions" do @@ -208,8 +208,8 @@ class ApplicationTableHelperTest < ActionView::TestCase result = users_applications_permissions_links(@application, @grantee) - assert_not_includes result, "View permissions" - assert_includes result, "Update permissions" + assert_not_includes result, "View" + assert_includes result, "Update" end should "return an empty string when the user can do neither" do @@ -234,7 +234,7 @@ class ApplicationTableHelperTest < ActionView::TestCase grantee = create(:api_user) stubs(:current_user).returns(granter) - assert_includes api_users_applications_permissions_link(application, grantee), "Update permissions" + assert_includes api_users_applications_permissions_link(application, grantee), "Update" end end