Merge pull request #3206 from alphagov/reduce-applications-table-acti…

…on-links-layout

Reduce applications table action links layout

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;
+  }
+}

app/helpers/application_table_helper.rb

@@ -1,10 +1,6 @@
 module ApplicationTableHelper
   include Pundit::Authorization
 
-  def wrap_links_in_actions_markup(links)
-    "<div class=\"govuk-table__actions\">#{links.join}</div>".html_safe
-  end
-
   def account_applications_grant_access_link(application)
     if policy([:account, Doorkeeper::Application]).grant_signin_permission?
       grant_access_link(application)
@@ -73,7 +69,12 @@ 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) }
+    ) do
+      button_or_link_content(
+        visible_text: "Grant",
+        visually_hidden_text: "access to #{application.name}",
+      )
+    end
   end
 
   def remove_access_link(application, user = nil)
@@ -85,9 +86,14 @@ 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) }
+    ) do
+      button_or_link_content(
+        visible_text: "Remove",
+        visually_hidden_text: "access to #{application.name}",
+      )
+    end
   end
 
   def view_permissions_link(application, user = nil)
@@ -97,7 +103,12 @@ 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") do
+      button_or_link_content(
+        visible_text: "View",
+        visually_hidden_text: "permissions for #{application.name}",
+      )
+    end
   end
 
   def update_permissions_link(application, user = nil)
@@ -115,13 +126,18 @@ 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") do
+      button_or_link_content(
+        visible_text: "Update",
+        visually_hidden_text: "permissions for #{application.name}",
+      )
+    end
   end
 
-  def button_or_link_content(visible_text, visually_hidden_join_word, application_name)
+  def button_or_link_content(visible_text:, visually_hidden_text:)
     safe_join([
       visible_text,
-      content_tag(:span, " #{visually_hidden_join_word} #{application_name}", class: "govuk-visually-hidden"),
+      content_tag(:span, " #{visually_hidden_text.strip}", class: "govuk-visually-hidden"),
     ])
   end
 end

app/helpers/components/table_helper.rb

@@ -61,6 +61,7 @@ def header(str, opt = {})
 
       def cell(str, opt = {}, &block)
         classes = %w[govuk-table__cell]
+        classes << opt[:classes] if opt[:classes]
         classes << "govuk-table__cell--#{opt[:format]}" if opt[:format]
         classes << "govuk-table__cell--empty" unless str || block_given?
         str ||= "Not set"

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,

app/views/api_users/applications/index.html.erb

@@ -34,14 +34,14 @@
     caption: "Apps #{@api_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: "Permissions" }
     ],
     rows: @applications.map do |application|
     [
       { text: application.name },
       { text: application.description },
-      { text: wrap_links_in_actions_markup([api_users_applications_permissions_link(application, @api_user)]) }
+      { text: api_users_applications_permissions_link(application, @api_user) }
     ]
     end,
     vertical_on_small_screen: true,

app/views/components/_table.html.erb

@@ -45,17 +45,19 @@
             <% if cellindex == 0 && first_cell_is_header %>
               <%= t.header cell[:text], {
                 scope: "row",
+                classes: cell[:classes],
                 format: cell[:format]
               } %>
             <% elsif vertical_on_small_screen && head.any? %>
-              <%= t.cell nil, { format: cell[:format] } do %>
+              <%= t.cell nil, { classes: cell[:classes], format: cell[:format] } do %>
                 <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] %>
               <% end %>
             <% else %>
               <%= t.cell cell[:text], {
+                classes: cell[:classes],
                 format: cell[:format]
               } %>
             <% end %>

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,

docs/access_and_permissions.md

@@ -68,14 +68,13 @@ These dependencies determine whether a user can:
 ```mermaid
 flowchart TD
     A(Account::ApplicationsController#index) --authorize [:account, Doorkeeper::Application]--> B(Account::ApplicationPolicy#index?)
-    C(app/views/account/applications/index.html.erb) --wrap_links_in_actions_markup--> D(ApplicationTableHelper#wrap_links_in_actions_markup)
-    C --account_applications_permissions_links--> E(ApplicationTableHelper#account_applications_permissions_links)
-    E --policy([:account, application]).view_permissions?--> F(Account::ApplicationPolicy#view_permissions?)
-    E --policy([:account, application]).edit_permissions?--> G(Account::ApplicationPolicy#edit_permissions?)
-    C --account_applications_remove_access_link--> H(ApplicationTableHelper#account_applications_remove_access_link)
-    H --policy([:account, application]).remove_signin_permission?--> I(Account::ApplicationPolicy#remove_signin_permission?)
-    C --account_applications_grant_access_link--> J(ApplicationTableHelper#account_applications_grant_access_link)
-    J --policy([:account, Doorkeeper::Application]).grant_signin_permission?--> K(Account::ApplicationPolicy#grant_signin_permission?)
+    C(app/views/account/applications/index.html.erb) --account_applications_permissions_links--> D(ApplicationTableHelper#account_applications_permissions_links)
+    D --policy([:account, application]).view_permissions?--> E(Account::ApplicationPolicy#view_permissions?)
+    D --policy([:account, application]).edit_permissions?--> F(Account::ApplicationPolicy#edit_permissions?)
+    C --account_applications_remove_access_link--> G(ApplicationTableHelper#account_applications_remove_access_link)
+    G --policy([:account, application]).remove_signin_permission?--> H(Account::ApplicationPolicy#remove_signin_permission?)
+    C --account_applications_grant_access_link--> I(ApplicationTableHelper#account_applications_grant_access_link)
+    I --policy([:account, Doorkeeper::Application]).grant_signin_permission?--> J(Account::ApplicationPolicy#grant_signin_permission?)
 ```
 
 #### Account permissions show
@@ -239,18 +238,17 @@ These dependencies determine whether a user can:
 flowchart TD
     A(Users::ApplicationsController#index) --authorize [{ user: }], policy_class: Users::ApplicationPolicy--> B(Users::ApplicationPolicy#index?)
     B --Pundit.policy(current_user, user).edit?--> C(UserPolicy#edit?)
-    D(app/views/users/applications/index.html.erb) --wrap_links_in_actions_markup--> E(ApplicationTableHelper#wrap_links_in_actions_markup)
-    D --users_applications_permissions_links--> F(ApplicationTableHelper#users_applications_permissions_links)
-    F --Users::ApplicationPolicy.new(current_user, { application:, user: }).view_permissions?--> G(Users::ApplicationPolicy#view_permissions?)
-    G --Pundit.policy(current_user, user).edit?--> H(UserPolicy#edit?)
-    F --Users::ApplicationPolicy.new(current_user, { application:, user: }).edit_permissions?--> I(Users::ApplicationPolicy#edit_permissions?)
-    I --Pundit.policy(current_user, user).edit?--> H
-    D --users_applications_remove_access_link--> J(ApplicationTableHelper#users_applications_remove_access_link)
-    J --Users::ApplicationPolicy.new(current_user, { application:, user: }).remove_signin_permission?--> K(Users::ApplicationPolicy#remove_signin_permission?)
-    K --Pundit.policy(current_user, user).edit?--> H
-    D --users_applications_grant_access_link--> L(ApplicationTableHelper#users_applications_grant_access_link)
-    L --Users::ApplicationPolicy.new(current_user, { application:, user: }).grant_signin_permission?--> M(Users::ApplicationPolicy#grant_signin_permission?)
-    M --Pundit.policy(current_user, user).edit?--> H
+    D(app/views/users/applications/index.html.erb) --users_applications_permissions_links--> E(ApplicationTableHelper#users_applications_permissions_links)
+    E --Users::ApplicationPolicy.new(current_user, { application:, user: }).view_permissions?--> F(Users::ApplicationPolicy#view_permissions?)
+    F --Pundit.policy(current_user, user).edit?--> G(UserPolicy#edit?)
+    E --Users::ApplicationPolicy.new(current_user, { application:, user: }).edit_permissions?--> H(Users::ApplicationPolicy#edit_permissions?)
+    H --Pundit.policy(current_user, user).edit?--> G
+    D --users_applications_remove_access_link--> I(ApplicationTableHelper#users_applications_remove_access_link)
+    I --Users::ApplicationPolicy.new(current_user, { application:, user: }).remove_signin_permission?--> J(Users::ApplicationPolicy#remove_signin_permission?)
+    J --Pundit.policy(current_user, user).edit?--> G
+    D --users_applications_grant_access_link--> K(ApplicationTableHelper#users_applications_grant_access_link)
+    K --Users::ApplicationPolicy.new(current_user, { application:, user: }).grant_signin_permission?--> L(Users::ApplicationPolicy#grant_signin_permission?)
+    L --Pundit.policy(current_user, user).edit?--> G
 ```
 
 #### Users permissions show