From e731dd70d174318b8efadc5bfaf7c46136f93b16 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 24 Jun 2019 16:02:49 +0100 Subject: [PATCH] Use chevrons not slashes to separate folders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It looks weird to have two different visual treatments for showing a navigable hierarchy. I reckon losing the slash won’t make things less folder like – Windows for example uses chevrons as foler separators. --- .../stylesheets/components/message.scss | 40 +++++++---- app/models/service.py | 5 +- app/templates/components/folder-path.html | 2 +- .../views/templates/_template_list.html | 19 +++-- app/templates/views/templates/copy.html | 2 +- tests/app/main/views/test_template_folders.py | 71 +++++++++---------- tests/app/main/views/test_templates.py | 32 ++++----- tests/app/models/test_service.py | 10 +-- 8 files changed, 105 insertions(+), 76 deletions(-) diff --git a/app/assets/stylesheets/components/message.scss b/app/assets/stylesheets/components/message.scss index 7923f28ef..20dbab56d 100644 --- a/app/assets/stylesheets/components/message.scss +++ b/app/assets/stylesheets/components/message.scss @@ -1,3 +1,29 @@ +@mixin separator { + display: inline-block; + vertical-align: top; + width: 20px; + height: $gutter; + position: relative; + + &:before { + content: ""; + display: block; + position: absolute; + top: -5px; + bottom: 1px; + right: 7px; + width: 9px; + height: 9px; + margin: auto 0; + -webkit-transform: rotate(45deg); + -ms-transform: rotate(45deg); + transform: rotate(45deg); + border: solid; + border-width: 2px 2px 0 0; + border-color: $secondary-text-colour; + } +} + .message { &-name { @@ -28,11 +54,7 @@ } &-separator { - display: inline-block; - vertical-align: top; - color: $secondary-text-colour; - padding: 0 4px 0 5px; - font-weight: normal; + @include separator; } } @@ -235,13 +257,7 @@ } &-separator { - - display: inline-block; - vertical-align: top; - color: $secondary-text-colour; - padding: 0 4px 0 5px; - font-weight: normal; - + @include separator; } &-manage-link { diff --git a/app/models/service.py b/app/models/service.py index c606f907f..3a45f6a17 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -452,7 +452,10 @@ class Service(JSONModel): "users_with_permission": folder["users_with_permission"] } while folder_attrs["parent_id"] is not None: - folder_attrs["name"] = parent["name"] + " / " + folder_attrs["name"] + folder_attrs["name"] = [ + parent["name"], + folder_attrs["name"], + ] if parent["parent_id"] is None: folder_attrs["parent_id"] = None else: diff --git a/app/templates/components/folder-path.html b/app/templates/components/folder-path.html index b183c84ea..5400e91c2 100644 --- a/app/templates/components/folder-path.html +++ b/app/templates/components/folder-path.html @@ -79,5 +79,5 @@ {% macro folder_path_separator() %} - / + {% endmacro %} diff --git a/app/templates/views/templates/_template_list.html b/app/templates/views/templates/_template_list.html index b2df718aa..a5e9cca93 100644 --- a/app/templates/views/templates/_template_list.html +++ b/app/templates/views/templates/_template_list.html @@ -1,6 +1,17 @@ {% from "components/checkbox.html" import unlabelled_checkbox %} {% from "components/message-count-label.html" import folder_contents_count, message_count_label %} +{% macro format_item_name(name) -%} + {%- if name is string -%} + {{- name -}} + {%- else -%} + {%- for part in name -%} + {{- format_item_name(part) -}} + {%- if not loop.last %} {% endif -%} + {%- endfor -%} + {% endif %} +{%- endmacro %} + {% if template_list.template_folder_id and not template_list.templates_to_show %}

{% if template_list.folder_is_empty %} @@ -24,16 +35,16 @@

{% for ancestor in item.ancestors %} - {{ ancestor.name }} - / + {{- format_item_name(ancestor.name) -}} + {% endfor %} {% if item.is_folder %} - {{ item.name }} + {{ format_item_name(item.name) }} {% else %} - {{ item.name }} + {{ format_item_name(item.name) }} {% endif %}

diff --git a/app/templates/views/templates/copy.html b/app/templates/views/templates/copy.html index 22a5aee6e..84c5a36ed 100644 --- a/app/templates/views/templates/copy.html +++ b/app/templates/views/templates/copy.html @@ -30,7 +30,7 @@ {% endif %} {{ ancestor.name }} - / + {% endfor %} {% if item.is_service %} diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 198649354..c93e70662 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -55,11 +55,11 @@ def _folder(name, folder_id=None, parent=None, users_with_permission=None): ['Email', 'Text message', 'Letter'], [ 'folder_one 2 folders', - 'folder_one / folder_one_one 1 template, 1 folder', - 'folder_one / folder_one_one / folder_one_one_one 1 template', - 'folder_one / folder_one_one / folder_one_one_one / sms_template_nested Text message template', - 'folder_one / folder_one_one / letter_template_nested Letter template', - 'folder_one / folder_one_two Empty', + 'folder_one folder_one_one 1 template, 1 folder', + 'folder_one folder_one_one folder_one_one_one 1 template', + 'folder_one folder_one_one folder_one_one_one sms_template_nested Text message template', + 'folder_one folder_one_one letter_template_nested Letter template', + 'folder_one folder_one_two Empty', 'folder_two Empty', 'sms_template_one Text message template', 'sms_template_two Text message template', @@ -103,9 +103,9 @@ def _folder(name, folder_id=None, parent=None, users_with_permission=None): ['All', 'Email', 'Letter'], [ 'folder_one 1 folder', - 'folder_one / folder_one_one 1 folder', - 'folder_one / folder_one_one / folder_one_one_one 1 template', - 'folder_one / folder_one_one / folder_one_one_one / sms_template_nested Text message template', + 'folder_one folder_one_one 1 folder', + 'folder_one folder_one_one folder_one_one_one 1 template', + 'folder_one folder_one_one folder_one_one_one sms_template_nested Text message template', 'sms_template_one Text message template', 'sms_template_two Text message template', ], @@ -126,15 +126,15 @@ def _folder(name, folder_id=None, parent=None, users_with_permission=None): ), ( 'folder_one – Templates – service one – GOV.UK Notify', - 'Templates / folder_one', + 'Templates folder_one', [{'template_type': 'all'}], {'template_folder_id': PARENT_FOLDER_ID}, ['Email', 'Text message', 'Letter'], [ 'folder_one_one 1 template, 1 folder', - 'folder_one_one / folder_one_one_one 1 template', - 'folder_one_one / folder_one_one_one / sms_template_nested Text message template', - 'folder_one_one / letter_template_nested Letter template', + 'folder_one_one folder_one_one_one 1 template', + 'folder_one_one folder_one_one_one sms_template_nested Text message template', + 'folder_one_one letter_template_nested Letter template', 'folder_one_two Empty', ], [ @@ -152,14 +152,14 @@ def _folder(name, folder_id=None, parent=None, users_with_permission=None): ), ( 'folder_one – Templates – service one – GOV.UK Notify', - 'Templates / folder_one', + 'Templates folder_one', [{'template_type': 'sms'}], {'template_type': 'sms', 'template_folder_id': PARENT_FOLDER_ID}, ['All', 'Email', 'Letter'], [ 'folder_one_one 1 folder', - 'folder_one_one / folder_one_one_one 1 template', - 'folder_one_one / folder_one_one_one / sms_template_nested Text message template', + 'folder_one_one folder_one_one_one 1 template', + 'folder_one_one folder_one_one_one sms_template_nested Text message template', ], [ 'folder_one_one 1 folder', @@ -173,7 +173,7 @@ def _folder(name, folder_id=None, parent=None, users_with_permission=None): ), ( 'folder_one – Templates – service one – GOV.UK Notify', - 'Templates / folder_one', + 'Templates folder_one', [{'template_type': 'email'}], {'template_type': 'email', 'template_folder_id': PARENT_FOLDER_ID}, ['All', 'Text message', 'Letter'], @@ -184,7 +184,7 @@ def _folder(name, folder_id=None, parent=None, users_with_permission=None): ), ( 'folder_one_one – folder_one – Templates – service one – GOV.UK Notify', - 'Templates / folder_one / folder_one_one', + 'Templates folder_one folder_one_one', [ {'template_type': 'all'}, {'template_type': 'all', 'template_folder_id': PARENT_FOLDER_ID}, @@ -193,7 +193,7 @@ def _folder(name, folder_id=None, parent=None, users_with_permission=None): ['Email', 'Text message', 'Letter'], [ 'folder_one_one_one 1 template', - 'folder_one_one_one / sms_template_nested Text message template', + 'folder_one_one_one sms_template_nested Text message template', 'letter_template_nested Letter template', ], [ @@ -209,7 +209,7 @@ def _folder(name, folder_id=None, parent=None, users_with_permission=None): ), ( 'folder_one_one_one – folder_one_one – folder_one – Templates – service one – GOV.UK Notify', - 'Templates / folder_one / folder_one_one / folder_one_one_one', + 'Templates folder_one folder_one_one folder_one_one_one', [ {'template_type': 'all'}, {'template_type': 'all', 'template_folder_id': PARENT_FOLDER_ID}, @@ -230,7 +230,7 @@ def _folder(name, folder_id=None, parent=None, users_with_permission=None): ), ( 'folder_one_one_one – folder_one_one – folder_one – Templates – service one – GOV.UK Notify', - 'Templates / folder_one / folder_one_one / folder_one_one_one', + 'Templates folder_one folder_one_one folder_one_one_one', [ {'template_type': 'email'}, {'template_type': 'email', 'template_folder_id': PARENT_FOLDER_ID}, @@ -248,7 +248,7 @@ def _folder(name, folder_id=None, parent=None, users_with_permission=None): ), ( 'folder_two – Templates – service one – GOV.UK Notify', - 'Templates / folder_two', + 'Templates folder_two', [{'template_type': 'all'}], {'template_folder_id': FOLDER_TWO_ID}, ['Email', 'Text message', 'Letter'], @@ -259,7 +259,7 @@ def _folder(name, folder_id=None, parent=None, users_with_permission=None): ), ( 'folder_two – Templates – service one – GOV.UK Notify', - 'Templates / folder_two', + 'Templates folder_two', [{'template_type': 'sms'}], {'template_folder_id': FOLDER_TWO_ID, 'template_type': 'sms'}, ['All', 'Email', 'Letter'], @@ -1405,17 +1405,17 @@ def test_show_custom_error_message( {}, [ ['folder_A', '1 template, 2 folders'], - ['folder_E / folder_F / folder_G', '1 template'], + ['folder_E folder_F folder_G', '1 template'], ['email_template_root', 'Email template'], ], [ ['folder_A', '1 template, 2 folders'], - ['folder_A', '/', 'folder_C', '1 template'], - ['folder_A', '/', 'folder_C', '/', 'sms_template_C', 'Text message template'], - ['folder_A', '/', 'folder_D', 'Empty'], - ['folder_A', '/', 'sms_template_A', 'Text message template'], - ['folder_E / folder_F / folder_G', '1 template'], - ['folder_E / folder_F / folder_G', '/', 'email_template_G', 'Email template'], + ['folder_A', 'folder_C', '1 template'], + ['folder_A', 'folder_C', 'sms_template_C', 'Text message template'], + ['folder_A', 'folder_D', 'Empty'], + ['folder_A', 'sms_template_A', 'Text message template'], + ['folder_E folder_F folder_G', '1 template'], + ['folder_E folder_F folder_G', 'email_template_G', 'Email template'], ['email_template_root', 'Email template'], ], None, @@ -1423,12 +1423,12 @@ def test_show_custom_error_message( ( {'template_type': 'email'}, [ - ['folder_E / folder_F / folder_G', '1 template'], + ['folder_E folder_F folder_G', '1 template'], ['email_template_root', 'Email template'], ], [ - ['folder_E / folder_F / folder_G', '1 template'], - ['folder_E / folder_F / folder_G', '/', 'email_template_G', 'Email template'], + ['folder_E folder_F folder_G', '1 template'], + ['folder_E folder_F folder_G', 'email_template_G', 'Email template'], ['email_template_root', 'Email template'], ], None, @@ -1440,9 +1440,9 @@ def test_show_custom_error_message( ], [ ['folder_A', '1 template, 1 folder'], - ['folder_A', '/', 'folder_C', '1 template'], - ['folder_A', '/', 'folder_C', '/', 'sms_template_C', 'Text message template'], - ['folder_A', '/', 'sms_template_A', 'Text message template'], + ['folder_A', 'folder_C', '1 template'], + ['folder_A', 'folder_C', 'sms_template_C', 'Text message template'], + ['folder_A', 'sms_template_A', 'Text message template'], ], None, ), @@ -1521,7 +1521,6 @@ def test_should_filter_templates_folder_page_based_on_user_permissions( and 'template-list-item' in tag['class'] and 'template-list-item-hidden-by-default' not in tag['class'] )) - assert [ [i.strip() for i in e.text.split("\n") if i.strip()] for e in displayed_page_items diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 4dafca534..a87863940 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -428,7 +428,7 @@ def test_user_with_only_send_and_view_sees_letter_page( _test_page_title=False, ) assert normalize_spaces(page.select_one('h1').text) == ( - 'Templates / Two week reminder' + 'Templates Two week reminder' ) assert normalize_spaces(page.select_one('title').text) == ( 'Two week reminder – Templates – service one – GOV.UK Notify' @@ -624,7 +624,7 @@ def test_should_be_able_to_view_a_template_with_links( ) assert normalize_spaces(page.select_one('h1').text) == ( - 'Templates / Two week reminder' + 'Templates Two week reminder' ) assert normalize_spaces(page.select_one('title').text) == ( 'Two week reminder – Templates – service one – GOV.UK Notify' @@ -855,27 +855,27 @@ def test_choose_a_template_to_copy( '6 templates' ), ( - 'Service 1 / sms_template_one ' + 'Service 1 sms_template_one ' 'Text message template' ), ( - 'Service 1 / sms_template_two ' + 'Service 1 sms_template_two ' 'Text message template' ), ( - 'Service 1 / email_template_one ' + 'Service 1 email_template_one ' 'Email template' ), ( - 'Service 1 / email_template_two ' + 'Service 1 email_template_two ' 'Email template' ), ( - 'Service 1 / letter_template_one ' + 'Service 1 letter_template_one ' 'Letter template' ), ( - 'Service 1 / letter_template_two ' + 'Service 1 letter_template_two ' 'Letter template' ), ( @@ -883,27 +883,27 @@ def test_choose_a_template_to_copy( '6 templates' ), ( - 'Service 2 / sms_template_one ' + 'Service 2 sms_template_one ' 'Text message template' ), ( - 'Service 2 / sms_template_two ' + 'Service 2 sms_template_two ' 'Text message template' ), ( - 'Service 2 / email_template_one ' + 'Service 2 email_template_one ' 'Email template' ), ( - 'Service 2 / email_template_two ' + 'Service 2 email_template_two ' 'Email template' ), ( - 'Service 2 / letter_template_one ' + 'Service 2 letter_template_one ' 'Letter template' ), ( - 'Service 2 / letter_template_two ' + 'Service 2 letter_template_two ' 'Letter template' ), ] @@ -1026,7 +1026,7 @@ def test_choose_a_template_to_copy_from_folder_within_service( ) assert normalize_spaces(page.select_one('.folder-heading').text) == ( - 'service one / Parent folder' + 'service one Parent folder' ) breadcrumb_links = page.select('.folder-heading a') assert len(breadcrumb_links) == 1 @@ -1046,7 +1046,7 @@ def test_choose_a_template_to_copy_from_folder_within_service( '1 template' ), ( - 'Child folder non-empty / Should appear in list (nested) ' + 'Child folder non-empty Should appear in list (nested) ' 'Text message template' ), ( diff --git a/tests/app/models/test_service.py b/tests/app/models/test_service.py index f71c89b8a..d24ed8821 100644 --- a/tests/app/models/test_service.py +++ b/tests/app/models/test_service.py @@ -80,13 +80,13 @@ def test_get_user_template_folders_only_returns_folders_visible_to_user( result = service.get_user_template_folders(User(active_user_with_permissions)) assert result == [ { - 'name': "Parent 1 - invisible / 1's Visible child", + 'name': ["Parent 1 - invisible", "1's Visible child"], 'id': mocker.ANY, 'parent_id': None, 'users_with_permission': [active_user_with_permissions['id']], }, { - 'name': "Parent 1 - invisible / 1's Invisible child / 1's Visible grandchild", + 'name': ["Parent 1 - invisible", ["1's Invisible child", "1's Visible grandchild"]], 'id': mocker.ANY, 'parent_id': None, 'users_with_permission': [active_user_with_permissions['id']], @@ -98,7 +98,7 @@ def test_get_user_template_folders_only_returns_folders_visible_to_user( 'users_with_permission': [active_user_with_permissions['id']], }, { - 'name': "2's Invisible child / 2's Visible grandchild", + 'name': ["2's Invisible child", "2's Visible grandchild"], 'id': mocker.ANY, 'parent_id': VIS_PARENT_FOLDER_ID, 'users_with_permission': [active_user_with_permissions['id']], @@ -124,13 +124,13 @@ def test_get_template_folders_shows_user_folders_when_user_id_passed_in( result = service.get_template_folders(user=User(active_user_with_permissions)) assert result == [ { - 'name': "Parent 1 - invisible / 1's Visible child", + 'name': ["Parent 1 - invisible", "1's Visible child"], 'id': mocker.ANY, 'parent_id': None, 'users_with_permission': [active_user_with_permissions['id']] }, { - 'name': "Parent 1 - invisible / 1's Invisible child / 1's Visible grandchild", + 'name': ["Parent 1 - invisible", ["1's Invisible child", "1's Visible grandchild"]], 'id': mocker.ANY, 'parent_id': None, 'users_with_permission': [active_user_with_permissions['id']]