From 5ac2abb7bcb0e33e112ac90bafb14beb47088fe3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 23 Apr 2019 10:21:19 +0100 Subject: [PATCH 1/7] Make center column of table wider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Most of the content of our ‘settings’ tables is in the value, not the key. The value is in the middle column. So we should allocate the most space to the value. The previous layout was based on the premise that most pages divided the grid like this: ``` _______ _______ _______ _______ _______ _______ _______ _______ | 1/8 | 1/8 | 1/8 | 1/8 | 1/8 | 1/8 | 1/8 | 1/8 | | | | | | | 2/8 | 2/8 | 2/8 | 2/8 | | | | | | |–Navigation––––|–Main column–––––––––––––––––––––––––––––––––––| | | | | | | 3/8 | 3/8 | | | | | | |–Label–––––––––––––––––|–Value––––––––––––Link–| | | | | |_______________|_______________________|_______________________| ``` This was because a lot of pages had a left column for emails, and a right column for text messages, so it felt consistent for tables to always default to 50% of the width of the main column. This consistency has faded with time, especially as we added letters. So this commit changes these tables to allocate more space to the central column, but still sticking to the grid like this: ``` _______ _______ _______ _______ _______ _______ _______ _______ | 1/8 | 1/8 | 1/8 | 1/8 | 1/8 | 1/8 | 1/8 | 1/8 | | | | | | | | | | | 2/8 | 2/8 | 4/8 | | | | | |–Navigation––––|–Main column–––––––––––––––––––––––––––––––––––| | | | | | | | 2/8 | 3/8 | 1/8 | | | | | | | |–Label–––––––––|–Value–––––––––––––––––|–––Link| |_______________|_______________|_______________________|_______| ``` Since there’s more space to display the value of a setting this commit also truncates settings that are too long to fit in the width of the column (for example a long email address) rather than the previous behaviour of truncating them. This all just makes things look a bit cleaner. --- app/assets/stylesheets/components/table.scss | 38 +++++++++++++++++++ .../organisation/settings/index.html | 2 +- app/templates/views/service-settings.html | 2 +- 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/app/assets/stylesheets/components/table.scss b/app/assets/stylesheets/components/table.scss index cd9b1643f..f47160917 100644 --- a/app/assets/stylesheets/components/table.scss +++ b/app/assets/stylesheets/components/table.scss @@ -51,6 +51,44 @@ } +.settings-table { + + table { + table-layout: fixed; + } + + th { + + &.table-field-heading-first { + width: 35%; // 33.33% + fudge + } + + &:last-child { + width: 17.5% // 16.67% + fudge + } + + } + + td { + + &:first-child { + + div { + white-space: normal; + } + + } + + div { + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; + } + + } + +} + %table-field, .table-field { diff --git a/app/templates/views/organisations/organisation/settings/index.html b/app/templates/views/organisations/organisation/settings/index.html index 59739904b..f1f6c5a54 100644 --- a/app/templates/views/organisations/organisation/settings/index.html +++ b/app/templates/views/organisations/organisation/settings/index.html @@ -29,7 +29,7 @@ {% if current_user.platform_admin %}

Platform admin settings

-
+
{% call mapping_table( caption='Platform admin settings', field_headings=['Label', 'Value', 'Action'], diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 6b81af730..91cb71aed 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -10,7 +10,7 @@

Settings

-
+
{% call mapping_table( caption='General', From 7c8ce6c62ab5c20e0ab1e46d46016605d2c24a64 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 23 Apr 2019 10:31:32 +0100 Subject: [PATCH 2/7] Make platform admin settings table body copy size No reason why it should be smaller and harder to read than the user-facing settings table. --- app/templates/views/service-settings.html | 174 +++++++++++----------- 1 file changed, 89 insertions(+), 85 deletions(-) diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 91cb71aed..ec7cec666 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -278,98 +278,102 @@ {% if current_user.platform_admin %} -

Platform admin settings

- {% call mapping_table( - caption='Settings', - field_headings=['Label', 'Value', 'Action'], - field_headings_visible=False, - caption_visible=False - ) %} +
- {% call row() %} - {{ text_field('Live')}} - {% if current_service.trial_mode and not current_service.organisation %} - {{ text_field('No (you need to assign this service to an organisation before you can make it live)') }} - {{ text_field('') }} - {% else %} - {{ boolean_field(not current_service.trial_mode) }} - {{ edit_field('Change', url_for('.service_switch_live', service_id=current_service.id)) }} - {% endif %} - {% endcall %} +

Platform admin settings

- {% call row() %} - {{ text_field('Count in list of live services')}} - {{ text_field('Yes' if current_service.count_as_live else 'No') }} - {{ edit_field('Change', url_for('.service_switch_count_as_live', service_id=current_service.id)) }} - {% endcall %} + {% call mapping_table( + caption='Settings', + field_headings=['Label', 'Value', 'Action'], + field_headings_visible=False, + caption_visible=False + ) %} - {% call row() %} - {{ text_field('Organisation')}} - {{ optional_text_field(current_service.organisation.name) }} - {{ edit_field('Change', url_for('.link_service_to_organisation', service_id=current_service.id)) }} - {% endcall %} - {% call row() %} - {{ text_field('Organisation type')}} - {{ optional_text_field( - (current_service.organisation_type or '')|title - ) }} - {{ edit_field('Change', url_for('.set_organisation_type', service_id=current_service.id)) }} - {% endcall %} - {% call row() %} - {{ text_field('Free text message allowance')}} - {{ text_field('{:,}'.format(current_service.free_sms_fragment_limit)) }} - {{ edit_field('Change', url_for('.set_free_sms_allowance', service_id=current_service.id)) }} - {% endcall %} - {% call row() %} - {{ text_field('Email branding' )}} - {{ text_field(current_service.email_branding_name) }} - {{ edit_field('Change', url_for('.service_set_email_branding', service_id=current_service.id)) }} - {% endcall %} - {% call row() %} - {{ text_field('Letter branding')}} - {{ optional_text_field(current_service.letter_branding.name) }} - {{ edit_field('Change', url_for('.service_set_letter_branding', service_id=current_service.id)) }} - {% endcall %} - {% call row() %} - {{ text_field('Data retention')}} - {% call field() %} - {{ current_service.data_retention | join(', ', attribute='notification_type') }} + {% call row() %} + {{ text_field('Live')}} + {% if current_service.trial_mode and not current_service.organisation %} + {{ text_field('No (you need to assign this service to an organisation before you can make it live)') }} + {{ text_field('') }} + {% else %} + {{ boolean_field(not current_service.trial_mode) }} + {{ edit_field('Change', url_for('.service_switch_live', service_id=current_service.id)) }} + {% endif %} {% endcall %} - {{ edit_field('Change', url_for('.data_retention', service_id=current_service.id)) }} + + {% call row() %} + {{ text_field('Count in list of live services')}} + {{ text_field('Yes' if current_service.count_as_live else 'No') }} + {{ edit_field('Change', url_for('.service_switch_count_as_live', service_id=current_service.id)) }} + {% endcall %} + + {% call row() %} + {{ text_field('Organisation')}} + {{ optional_text_field(current_service.organisation.name) }} + {{ edit_field('Change', url_for('.link_service_to_organisation', service_id=current_service.id)) }} + {% endcall %} + {% call row() %} + {{ text_field('Organisation type')}} + {{ optional_text_field( + (current_service.organisation_type or '')|title + ) }} + {{ edit_field('Change', url_for('.set_organisation_type', service_id=current_service.id)) }} + {% endcall %} + {% call row() %} + {{ text_field('Free text message allowance')}} + {{ text_field('{:,}'.format(current_service.free_sms_fragment_limit)) }} + {{ edit_field('Change', url_for('.set_free_sms_allowance', service_id=current_service.id)) }} + {% endcall %} + {% call row() %} + {{ text_field('Email branding' )}} + {{ text_field(current_service.email_branding_name) }} + {{ edit_field('Change', url_for('.service_set_email_branding', service_id=current_service.id)) }} + {% endcall %} + {% call row() %} + {{ text_field('Letter branding')}} + {{ optional_text_field(current_service.letter_branding.name) }} + {{ edit_field('Change', url_for('.service_set_letter_branding', service_id=current_service.id)) }} + {% endcall %} + {% call row() %} + {{ text_field('Data retention')}} + {% call field() %} + {{ current_service.data_retention | join(', ', attribute='notification_type') }} + {% endcall %} + {{ edit_field('Change', url_for('.data_retention', service_id=current_service.id)) }} + {% endcall %} + + {% for permission in service_permissions %} + {% if not service_permissions[permission].requires or current_service.has_permission(service_permissions[permission].requires) %} + {% call row() %} + {{ text_field(service_permissions[permission].title)}} + {{ boolean_field(current_service.has_permission(permission)) }} + {{ edit_field('Change', url_for(service_permissions[permission].endpoint or '.service_set_permission', service_id=current_service.id, permission=permission if not service_permissions[permission].endpoint else None)) }} + {% endcall %} + {% endif %} + {% endfor %} + {% endcall %} - {% for permission in service_permissions %} - {% if not service_permissions[permission].requires or current_service.has_permission(service_permissions[permission].requires) %} - {% call row() %} - {{ text_field(service_permissions[permission].title)}} - {{ boolean_field(current_service.has_permission(permission)) }} - {{ edit_field('Change', url_for(service_permissions[permission].endpoint or '.service_set_permission', service_id=current_service.id, permission=permission if not service_permissions[permission].endpoint else None)) }} - {% endcall %} + +
{% endif %} From d3caaf94b1b2752b9b30b38c0745d88960bd0593 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 23 Apr 2019 10:42:37 +0100 Subject: [PATCH 3/7] Make archive/suspend links not buttons MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit They don’t immediately perform an action, so semantically they are links, not buttons. --- app/templates/views/service-settings.html | 29 ++++++++++--------- .../test_service_setting_permissions.py | 27 ++++++++++++----- 2 files changed, 36 insertions(+), 20 deletions(-) diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index ec7cec666..3332474f3 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -353,26 +353,29 @@ {% endcall %} - +

{% endif %} diff --git a/tests/app/main/views/service_settings/test_service_setting_permissions.py b/tests/app/main/views/service_settings/test_service_setting_permissions.py index 7f656e66a..ff3f327e9 100644 --- a/tests/app/main/views/service_settings/test_service_setting_permissions.py +++ b/tests/app/main/views/service_settings/test_service_setting_permissions.py @@ -92,16 +92,29 @@ def test_service_setting_toggles_show(get_service_settings_page, service_one, se assert normalize_spaces(page.find('a', {'href': button_url}).find_parent('tr').text.strip()) == text -@pytest.mark.parametrize('service_fields, endpoint, kwargs, text', [ - ({'active': True}, '.archive_service', {}, 'Archive service'), - ({'active': True}, '.suspend_service', {}, 'Suspend service'), - ({'active': False}, '.resume_service', {}, 'Resume service'), +@pytest.mark.parametrize('service_fields, endpoint, index, text', [ + ({'active': True}, '.archive_service', 0, 'Archive service'), + ({'active': True}, '.suspend_service', 1, 'Suspend service'), + ({'active': False}, '.resume_service', 0, 'Resume service'), + pytest.param( + {'active': False}, '.archive_service', 1, 'Resume service', + marks=pytest.mark.xfail(raises=IndexError) + ) ]) -def test_service_setting_button_toggles(get_service_settings_page, service_one, service_fields, endpoint, kwargs, text): - button_url = url_for(endpoint, **kwargs, service_id=service_one['id']) +def test_service_setting_button_toggles( + get_service_settings_page, + service_one, + service_fields, + endpoint, + index, + text, +): + button_url = url_for(endpoint, service_id=service_one['id']) service_one.update(service_fields) page = get_service_settings_page() - assert normalize_spaces(page.find('a', {'class': 'button', 'href': button_url}).text.strip()) == text + link = page.select('.page-footer-delete-link a')[index] + assert normalize_spaces(link.text) == text + assert link['href'] == button_url @pytest.mark.parametrize('permissions,permissions_text,visible', [ From da19c0b4ad97fb9089e6988bb97488e0cf2e6702 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 23 Apr 2019 11:26:36 +0100 Subject: [PATCH 4/7] Remove bullets from lists in tables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I think this is something we inherited from the Digital Marketplace code. We only use this for organisation settings are the moment, but the list markers are redundant because each item will never wrap onto a new line; it will truncate instead. Still keeps a little sliver of spacing just so it doesn’t look like a paragraph. --- app/assets/stylesheets/components/table.scss | 4 ++++ app/templates/components/table.html | 2 +- .../views/organisations/organisation/settings/index.html | 2 +- tests/app/main/views/test_send.py | 6 +++--- 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/app/assets/stylesheets/components/table.scss b/app/assets/stylesheets/components/table.scss index f47160917..c757c18da 100644 --- a/app/assets/stylesheets/components/table.scss +++ b/app/assets/stylesheets/components/table.scss @@ -85,6 +85,10 @@ text-overflow: ellipsis; } + ul li { + margin-bottom: 5px; + } + } } diff --git a/app/templates/components/table.html b/app/templates/components/table.html index c5708512c..c5171c02a 100644 --- a/app/templates/components/table.html +++ b/app/templates/components/table.html @@ -90,7 +90,7 @@ {% macro text_field(text, status='', truncate=false) -%} {% call field(status=status) %} {% if text is iterable and text is not string %} -
    +
      {% for item in text %} {% if item %}
    • {{ item }}
      • diff --git a/app/templates/views/organisations/organisation/settings/index.html b/app/templates/views/organisations/organisation/settings/index.html index f1f6c5a54..a08f70d32 100644 --- a/app/templates/views/organisations/organisation/settings/index.html +++ b/app/templates/views/organisations/organisation/settings/index.html @@ -29,7 +29,7 @@ {% if current_user.platform_admin %}

      Platform admin settings

      -
      +
      {% call mapping_table( caption='Platform admin settings', field_headings=['Label', 'Value', 'Action'], diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 3fa335965..5f0cc7a37 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -661,7 +661,7 @@ def test_upload_valid_csv_shows_preview_and_table( ( ' ' '
      ' - '
        ' + '
          ' '
        • foo
        • foo
        • foo
          • ' '
        ' '
      ' @@ -674,7 +674,7 @@ def test_upload_valid_csv_shows_preview_and_table( ( ' ' '
      ' - '
        ' + '
          ' '
        • foo
        • foo
        • foo
          • ' '
        ' '
      ' @@ -687,7 +687,7 @@ def test_upload_valid_csv_shows_preview_and_table( ( ' ' '
      ' - '
        ' + '
          ' '
        • foo
        • foo
          • ' '
        ' '
      ' From a1025111d9a59508328cffcdc19e1b290fb192a4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 26 Apr 2019 11:43:41 +0100 Subject: [PATCH 5/7] Allow overflow on right-aligned table cells MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The right aligned cells contain ‘Change’ links. These have a large `border` to make the clickable area bigger. This commit removes the `overflow: hidden` from these cells, so that the larger border remains visible. --- app/assets/stylesheets/components/table.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/stylesheets/components/table.scss b/app/assets/stylesheets/components/table.scss index c757c18da..0af72095f 100644 --- a/app/assets/stylesheets/components/table.scss +++ b/app/assets/stylesheets/components/table.scss @@ -69,7 +69,7 @@ } - td { + td.table-field-center-aligned { &:first-child { From 5340c4c2e8608cff0938901ad6bea1d7053df360 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 29 Apr 2019 16:15:55 +0100 Subject: [PATCH 6/7] Prefer CSS-native first child selector This fits nicer with the block below and with the CSS further down that applies certain styles for the first cell using `:first-child` too. --- app/assets/stylesheets/components/table.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/stylesheets/components/table.scss b/app/assets/stylesheets/components/table.scss index 0af72095f..d210d11d8 100644 --- a/app/assets/stylesheets/components/table.scss +++ b/app/assets/stylesheets/components/table.scss @@ -59,7 +59,7 @@ th { - &.table-field-heading-first { + &:first-child { width: 35%; // 33.33% + fudge } From 0512f40ad3d7fdb4d285a4f7e44659cca6f3e09f Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 29 Apr 2019 16:21:47 +0100 Subject: [PATCH 7/7] Fix misleading class name Not sure where this came from, but visually our tables have always aligned text to the left by default. --- app/assets/stylesheets/components/table.scss | 2 +- app/templates/components/table.html | 2 +- tests/app/main/views/test_dashboard.py | 2 +- tests/app/main/views/test_send.py | 18 +++++++++--------- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/app/assets/stylesheets/components/table.scss b/app/assets/stylesheets/components/table.scss index d210d11d8..0209b4d44 100644 --- a/app/assets/stylesheets/components/table.scss +++ b/app/assets/stylesheets/components/table.scss @@ -69,7 +69,7 @@ } - td.table-field-center-aligned { + td.table-field-left-aligned { &:first-child { diff --git a/app/templates/components/table.html b/app/templates/components/table.html index c5171c02a..656ef1c47 100644 --- a/app/templates/components/table.html +++ b/app/templates/components/table.html @@ -67,7 +67,7 @@ {% macro field(align='left', status='', border=True) -%} - {% set field_alignment = 'table-field-right-aligned' if align == 'right' else 'table-field-center-aligned' %} + {% set field_alignment = 'table-field-right-aligned' if align == 'right' else 'table-field-left-aligned' %} {% set border = '' if border else 'table-field-noborder' %} diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 46418d742..fd9aa1268 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -569,7 +569,7 @@ def test_monthly_shows_letters_in_breakdown( service_id=service_one['id'] ) - columns = page.select('.table-field-center-aligned .big-number-label') + columns = page.select('.table-field-left-aligned .big-number-label') assert normalize_spaces(columns[0].text) == 'emails' assert normalize_spaces(columns[1].text) == 'text messages' diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 5f0cc7a37..a59d64f32 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -656,10 +656,10 @@ def test_upload_valid_csv_shows_preview_and_table( for row_index, row in enumerate([ ( - '
      07700900001
      ', - '
      A
      ', + '
      07700900001
      ', + '
      A
      ', ( - ' ' + ' ' '
      ' '
        ' '
      • foo
      • foo
      • foo
        • ' @@ -669,10 +669,10 @@ def test_upload_valid_csv_shows_preview_and_table( ) ), ( - '
        07700900002
        ', - '
        B
        ', + '
        07700900002
        ', + '
        B
        ', ( - ' ' + ' ' '
        ' '
          ' '
        • foo
        • foo
        • foo
          • ' @@ -682,10 +682,10 @@ def test_upload_valid_csv_shows_preview_and_table( ) ), ( - '
          07700900003
          ', - '
          C
          ', + '
          07700900003
          ', + '
          C
          ', ( - ' ' + ' ' '
          ' '
            ' '
          • foo
          • foo
            • '