From be83ea36eae95e5ed1b707cd57446861216312b7 Mon Sep 17 00:00:00 2001
From: Tom Byers <tombaromba@gmail.com>
Date: Wed, 19 Aug 2020 14:52:17 +0100
Subject: [PATCH] Change list items on /users page

Change the HTML & CSS for user-list-item's to
support their content being split into 2 columns
of a grid instead of the edit link being
positioned absolutely.

Also includes:
1. removes `<div>`s from `<ul>` (non-valid HTML)
2. split action link out from permissions list
3. split summary of folder permissions out from
   permissions list
4. introduces a class for blocks of text that
   appear when there are no items.
5. fixes tests broken by changes to HTML
---
 .../stylesheets/components/tick-cross.scss    | 36 +--------
 app/assets/stylesheets/views/users.scss       | 59 ++++++++++++--
 app/templates/views/manage-users.html         | 78 ++++++++++---------
 .../test_organisation_invites.py              |  2 +-
 4 files changed, 95 insertions(+), 80 deletions(-)

diff --git a/app/assets/stylesheets/components/tick-cross.scss b/app/assets/stylesheets/components/tick-cross.scss
index 9d9ce2489..ef04b0a93 100644
--- a/app/assets/stylesheets/components/tick-cross.scss
+++ b/app/assets/stylesheets/components/tick-cross.scss
@@ -36,44 +36,10 @@
 
     }
 
-    &-edit-link {
-
-      text-align: right;
-      position: absolute;
-      top: -25px;
-      right: -135px;
-
-      a {
-
-        &:before {
-          content: "";
-          display: block;
-
-          position: absolute;
-
-          top: -1 * govuk-spacing(3) - 1;
-          right: 0;
-          bottom: -1 * govuk-spacing(3);
-          left: -1 * govuk-spacing(3);
-
-          background: transparent;
-        }
-
-        &:active:before,
-        &:focus:before {
-          border-color: $yellow;
-          border-style: solid;
-          border-width: 15px 3px 15px 15px;
-          right: -3px;
-        }
-
-      }
-
-    }
-
     &-hint {
       color: #6F777B;
       padding-top: 5px;
+      margin: 0;
     }
 
   }
diff --git a/app/assets/stylesheets/views/users.scss b/app/assets/stylesheets/views/users.scss
index 4abef1993..a7b126c4a 100644
--- a/app/assets/stylesheets/views/users.scss
+++ b/app/assets/stylesheets/views/users.scss
@@ -7,9 +7,8 @@ $item-top-padding: govuk-spacing(3);
 
   &-item {
 
-    padding: $item-top-padding 150px govuk-spacing(3) 0;
+    padding: govuk-spacing(3) 0px;
     border-top: 1px solid $border-colour;
-    position: relative;
 
     &-heading {
 
@@ -29,13 +28,61 @@ $item-top-padding: govuk-spacing(3);
       border-bottom: 1px solid $border-colour;
     }
 
+    & :last-child {
+      margin-bottom: 0;
+    }
+
+    &-blank {
+
+      margin-bottom: govuk-spacing(3);
+
+      @include govuk-media-query($from: tablet) {
+        margin-bottom: 0;
+      }
+
+    }
+
   }
 
   &-edit-link {
-    text-align: right;
-    position: absolute;
-    top: $item-top-padding;
-    right: 0px;
+
+    position: relative;
+    display: block;
+    margin: govuk-spacing(3) 0 govuk-spacing(3);
+
+    @include govuk-media-query($from: tablet) {
+      float: right;
+      margin: 0;
+    }
+
+    &:before {
+      content: "";
+      display: block;
+
+      position: absolute;
+
+      top: -1 * govuk-spacing(3) - 1;
+      right: 0;
+      bottom: -1 * govuk-spacing(3);
+      left: -1 * govuk-spacing(3);
+
+      background: transparent;
+    }
+
+    &:active:before,
+    &:focus:before {
+      border-color: $yellow;
+      border-style: solid;
+      border-width: 15px 3px;
+      right: -3px;
+      left: -3px;
+
+      @include govuk-media-query($from: tablet) {
+        border-width: 15px 3px 15px 15px;
+        left: -15px;
+      }
+    }
+
   }
 
 }
diff --git a/app/templates/views/manage-users.html b/app/templates/views/manage-users.html
index 663d8ccb5..f8c9d858b 100644
--- a/app/templates/views/manage-users.html
+++ b/app/templates/views/manage-users.html
@@ -22,33 +22,35 @@
   <div class="user-list">
     {% for user in users %}
       <div class="user-list-item">
-        <h2 class="user-list-item-heading" title="{{ user.email_address }}">
-          {%- if user.name -%}
-            <span class="heading-small live-search-relevant">{{ user.name }}</span>&ensp;
-          {%- endif -%}
-          <span class="hint">
-            {%- if user.status == 'pending' -%}
-              <span class="live-search-relevant">{{ user.email_address }}</span> (invited)
-            {%- elif user.status == 'cancelled' -%}
-              <span class="live-search-relevant">{{ user.email_address }}</span> (cancelled invite)
-            {%- elif user.id == current_user.id -%}
-              <span class="live-search-relevant">(you)</span>
-            {% else %}
-              <span class="live-search-relevant">{{ user.email_address }}</span>
-            {% endif %}
-          </span>
-        </h2>
-        <ul class="tick-cross-list govuk-grid-row">
-          <div class="tick-cross-list-permissions govuk-grid-column-three-quarters">
-            {% for permission, label in permissions %}
-              {{ tick_cross(
-                user.has_permission_for_service(current_service.id, permission),
-                label
-              ) }}
-            {% endfor %}
+        <div class="govuk-grid-row">
+          <div class="govuk-grid-column-three-quarters">
+            <h2 class="user-list-item-heading" title="{{ user.email_address }}">
+              {%- if user.name -%}
+              <span class="heading-small live-search-relevant">{{ user.name }}</span>&ensp;
+              {%- endif -%}
+              <span class="hint">
+              {%- if user.status == 'pending' -%}
+                <span class="live-search-relevant">{{ user.email_address }}</span> (invited)
+              {%- elif user.status == 'cancelled' -%}
+                <span class="live-search-relevant">{{ user.email_address }}</span> (cancelled invite)
+              {%- elif user.id == current_user.id -%}
+                <span class="live-search-relevant">(you)</span>
+              {% else %}
+                <span class="live-search-relevant">{{ user.email_address }}</span>
+              {% endif %}
+              </span>
+            </h2>
+            <ul class="tick-cross-list-permissions">
+              {% for permission, label in permissions %}
+                {{ tick_cross(
+                  user.has_permission_for_service(current_service.id, permission),
+                  label
+                ) }}
+              {% endfor %}
+            </ul>
             {# only show if the service has folders #}
             {% if current_service.all_template_folders %}
-              <div class="tick-cross-list-hint">
+              <p class="govuk-body tick-cross-list-hint">
                 {% set folder_count = user.template_folders_for_service(current_service) | length %}
 
                 {% if folder_count == 0 %}
@@ -58,28 +60,28 @@
                 {% else %}
                   Can see all folders
                 {% endif%}
-              </div>
+              </p>
             {% endif %}
             {% if current_service.has_permission('email_auth') %}
-              <div class="tick-cross-list-hint">
+              <p class="govuk-body tick-cross-list-hint">
                 {% if user.auth_type == 'sms_auth' %}
                   Signs in with a text message code
                 {% else %}
                   Signs in with an email link
                 {% endif %}
-              </div>
+              </p>
             {% endif %}
-          </div>
-          {% if current_user.has_permissions('manage_service') %}
-            <li class="tick-cross-list-edit-link">
-              {% if user.status == 'pending' %}
-                <a class="govuk-link govuk-link--no-visited-state" href="{{ url_for('.cancel_invited_user', service_id=current_service.id, invited_user_id=user.id)}}">Cancel invitation<span class="govuk-visually-hidden"> for {{ user.email_address }}</span></a>
-              {% elif user.state == 'active' and current_user.id != user.id %}
-                <a class="govuk-link govuk-link--no-visited-state" href="{{ url_for('.edit_user_permissions', service_id=current_service.id, user_id=user.id)}}">Change details<span class="govuk-visually-hidden"> for {{ user.name }} {{ user.email_address }}</span></a>
+            </div>
+            <div class="govuk-grid-column-one-quarter">
+              {% if current_user.has_permissions('manage_service') %}
+                {% if user.status == 'pending' %}
+                  <a class="user-list-edit-link govuk-link govuk-link--no-visited-state" href="{{ url_for('.cancel_invited_user', service_id=current_service.id, invited_user_id=user.id)}}">Cancel invitation<span class="govuk-visually-hidden"> for {{ user.email_address }}</span></a>
+                {% elif user.state == 'active' and current_user.id != user.id %}
+                  <a class="user-list-edit-link govuk-link govuk-link--no-visited-state" href="{{ url_for('.edit_user_permissions', service_id=current_service.id, user_id=user.id)}}">Change details<span class="govuk-visually-hidden"> for {{ user.name }} {{ user.email_address }}</span></a>
+                {% endif %}
               {% endif %}
-            </li>
-          {% endif %}
-        </ul>
+            </div>
+          </div>
       </div>
     {% endfor %}
   </div>
diff --git a/tests/app/main/views/organisations/test_organisation_invites.py b/tests/app/main/views/organisations/test_organisation_invites.py
index d32748310..9a80301aa 100644
--- a/tests/app/main/views/organisations/test_organisation_invites.py
+++ b/tests/app/main/views/organisations/test_organisation_invites.py
@@ -28,7 +28,7 @@ def test_view_team_members(
         ) == 'Test User {}'.format(i + 1)
 
     assert normalize_spaces(
-        page.select('.tick-cross-list-edit-link')[0].text
+        page.select('.user-list-edit-link')[0].text
     ) == 'Cancel invitation'