From 88ad982bf70a614cd2580eb2e7ae481d7745fbf3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 30 Jan 2019 10:09:04 +0000 Subject: [PATCH] Improve display of folder path when deeply nested MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s a bit rudimentary to only show the current place in the hierarchy and the parent. You lose a sense of how deep you are. But we can’t just show the full path, because it can be arbitrarily long. So what this commit does is show the full path, but truncates the display of any items. Further-up than the current folder or its parent. This also helps disambiguate between folders and templates, because folders are always shown with the folder icon. This probably won’t affect many teams, because we don’t anticipate a lot of deep nesting. --- app/assets/images/folder-black-bold.png | Bin 0 -> 267 bytes app/assets/images/folder-black-bold.svg | 11 +++ app/assets/images/folder-blue-bold-hover.png | Bin 0 -> 267 bytes app/assets/images/folder-blue-bold-hover.svg | 11 +++ .../stylesheets/components/message.scss | 80 ++++++++++++++---- app/models/service.py | 8 +- app/templates/components/folder-path.html | 11 ++- .../views/templates/_template_list.html | 8 +- tests/app/main/views/test_template_folders.py | 44 +++++----- tests/app/main/views/test_templates.py | 2 +- 10 files changed, 125 insertions(+), 50 deletions(-) create mode 100644 app/assets/images/folder-black-bold.png create mode 100644 app/assets/images/folder-black-bold.svg create mode 100644 app/assets/images/folder-blue-bold-hover.png create mode 100644 app/assets/images/folder-blue-bold-hover.svg diff --git a/app/assets/images/folder-black-bold.png b/app/assets/images/folder-black-bold.png new file mode 100644 index 0000000000000000000000000000000000000000..e2eb5f75700b36a7603db8017d84925f2fd8af0b GIT binary patch literal 267 zcmeAS@N?(olHy`uVBq!ia0vp^Qa~)i!3HEJJLv8MQj#UE5hcO-X(i=}MX3yqDfvmM z3ZA)%>8U}fi7AzZCsS>Jiq?9%IEGZ*N_z6_;m`99G8G@bo)&jyGh!6x%o1(rD^j?w zBqnkA$`zgtUqca_+HX@5AGh%um`}(vS+spN5ur)C(ST7co zBB*!Z^y$+Np1k4tJe5J<{qjhepTF6S0=ObN0$I4O>F6Bv;+SJsE7jpIm@mkYGAU8; z@RpSY-7_R!u(;_;ZOyo%bc&I?sF3m0$tx@g#tR!Jwn&v_u*f OID@CFpUXO@geCykJYO0B literal 0 HcmV?d00001 diff --git a/app/assets/images/folder-black-bold.svg b/app/assets/images/folder-black-bold.svg new file mode 100644 index 000000000..c3017e3f5 --- /dev/null +++ b/app/assets/images/folder-black-bold.svg @@ -0,0 +1,11 @@ + + + + + + + diff --git a/app/assets/images/folder-blue-bold-hover.png b/app/assets/images/folder-blue-bold-hover.png new file mode 100644 index 0000000000000000000000000000000000000000..06f7865a6c47775589c95b75b2889199a609d8c3 GIT binary patch literal 267 zcmeAS@N?(olHy`uVBq!ia0vp^Qa~)i!3HEJJLv8MQj#UE5hcO-X(i=}MX3yqDfvmM z3ZA)%>8U}fi7AzZCsS>Jiq?9%IEGZ*O1kpS^3(iAiw_U7I(wCvXEt=QY~^xrx15kI z$lbG0`xN7mHxgWNb-xd9_ + + + + + + diff --git a/app/assets/stylesheets/components/message.scss b/app/assets/stylesheets/components/message.scss index b6c0b1dd0..8e73a8cc2 100644 --- a/app/assets/stylesheets/components/message.scss +++ b/app/assets/stylesheets/components/message.scss @@ -5,10 +5,13 @@ margin: 0; a { - display: inline; margin-bottom: -$gutter; padding-bottom: $gutter; + &:hover { + color: $link-hover-colour; + } + &:focus { // Use box shadow instead of outline to avoid buggy outline @@ -86,23 +89,35 @@ &-folder { - a:first-child { + display: inline-block; + padding-left: 40px; + background-image: file-url('folder-blue-bold.svg'); + background-repeat: no-repeat; + background-size: auto 20px; + background-position: 0px 2px; - display: inline-block; - text-indent: 40px; - background-image: file-url('folder-blue-bold.svg'); - background-repeat: no-repeat; - background-size: auto 20px; - background-position: 0px 2px; + @include ie-lte(8) { + background-image: file-url('folder-blue-bold.png'); + } + + &:hover { + + background-image: file-url('folder-blue-bold-hover.svg'); @include ie-lte(8) { - background-image: file-url('folder-blue-bold.png'); + background-image: file-url('folder-blue-bold-hover.png'); } } } + &-template { + a { + display: inline; + } + } + &-empty { color: $secondary-text-colour; padding: $gutter-half 0 $gutter-one-third 0; @@ -139,31 +154,60 @@ display: inline-block; vertical-align: top; - padding: 5px 0 0 0; background-repeat: no-repeat; - background-size: 38px auto; - background-position: 0 1px; + background-size: auto 20px; + background-position: 0px 2px; + min-height: 30px; } + &-folder { + + padding: 0 0 0 40px; + background-image: file-url('folder-black-bold.svg'); + + @include ie-lte(8) { + background-image: file-url('folder-black-bold.png'); + } + + } + + &-folder-truncated { + width: 0; + padding: 0 0 0 30px; + overflow: hidden; + } + + &-folder-root-truncated { + max-width: 1.4em; + overflow: hidden; + text-overflow: ellipsis; + } + a { - padding-top: 5px; display: inline-block; vertical-align: top; &.folder-heading-folder { - padding: 5px 0 0 60px; - background-image: file-url('folder-blue.svg'); + background-image: file-url('folder-blue-bold.svg'); max-width: 33%; overflow: hidden; text-overflow: ellipsis; @include ie-lte(8) { - background-image: file-url('folder-blue.png'); + background-image: file-url('folder-blue-bold.png'); } + &:hover { + background-image: file-url('folder-blue-bold-hover.svg'); + } + + } + + &:hover { + color: $link-hover-colour; } } @@ -173,7 +217,7 @@ display: inline-block; vertical-align: top; color: $secondary-text-colour; - padding: 5px 4px 0 5px; + padding: 0 4px 0 5px; font-weight: normal; } @@ -181,7 +225,7 @@ &-manage-link { display: block; text-align: right; - padding: ($gutter - 2px) 0 0 0; + padding: ($gutter - 7px) 0 0 0; } } diff --git a/app/models/service.py b/app/models/service.py index 4f1e9b23a..fbb5485d6 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -377,14 +377,12 @@ class Service(): if folder['id'] is None: return [folder] - return [ - self.get_template_folder(folder['parent_id']), - folder, + return self.get_template_folder_path(folder['parent_id']) + [ + self.get_template_folder(folder['id']) ] def get_template_path(self, template): - return [ - self.get_template_folder(template['folder']), + return self.get_template_folder_path(template['folder']) + [ template, ] diff --git a/app/templates/components/folder-path.html b/app/templates/components/folder-path.html index c19af80f9..776707ce0 100644 --- a/app/templates/components/folder-path.html +++ b/app/templates/components/folder-path.html @@ -14,13 +14,18 @@

{% for folder in folders %} {% if loop.last and not link_current_item %} - {{ folder.name }} + {% if folder.template_type or not folder.id %} + {{ folder.name }} + {% else %} + {{ folder.name }} + {% endif %} {% else %} {% if folder.id %} - {{ folder.name }} {% if not loop.last %}{{ folder_path_separator() }}{% endif %} + {{ folder.name }} {% else %} - Templates {% if not loop.last %}{{ folder_path_separator() }}{% endif %} + Templates {% endif %} + {% if not loop.last %}{{ folder_path_separator() }}{% endif %} {% endif %} {% endfor %}

diff --git a/app/templates/views/templates/_template_list.html b/app/templates/views/templates/_template_list.html index b1be5f719..42fe75105 100644 --- a/app/templates/views/templates/_template_list.html +++ b/app/templates/views/templates/_template_list.html @@ -21,18 +21,18 @@ value=item.id, ) }} {% endif %} -

+

{% for ancestor in item.ancestors %} - + {{ ancestor.name }} / {% endfor %} {% if item.is_folder %} - + {{ item.name }} {% else %} - + {{ item.name }} {% endif %} diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index fbde3fd8b..9bd254037 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -124,7 +124,7 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare ( 'Templates – service one – GOV.UK Notify', 'Templates', - None, + [], {}, ['Text message', 'Email', 'Letter'], [ @@ -172,7 +172,7 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare ( 'Templates – service one – GOV.UK Notify', 'Templates', - None, + [], {'template_type': 'sms'}, ['All', 'Email', 'Letter'], [ @@ -201,7 +201,7 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare ( 'folder_one – Templates – service one – GOV.UK Notify', 'Templates / folder_one', - {'template_type': 'all'}, + [{'template_type': 'all'}], {'template_folder_id': PARENT_FOLDER_ID}, ['Text message', 'Email', 'Letter'], [ @@ -227,7 +227,7 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare ( 'folder_one – Templates – service one – GOV.UK Notify', 'Templates / folder_one', - {'template_type': 'sms'}, + [{'template_type': 'sms'}], {'template_type': 'sms', 'template_folder_id': PARENT_FOLDER_ID}, ['All', 'Email', 'Letter'], [ @@ -248,7 +248,7 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare ( 'folder_one – Templates – service one – GOV.UK Notify', 'Templates / folder_one', - {'template_type': 'email'}, + [{'template_type': 'email'}], {'template_type': 'email', 'template_folder_id': PARENT_FOLDER_ID}, ['All', 'Text message', 'Letter'], [], @@ -257,9 +257,12 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare 'There are no email templates in this folder', ), ( - 'folder_one_one – folder_one – service one – GOV.UK Notify', - 'folder_one / folder_one_one', - {'template_type': 'all', 'template_folder_id': PARENT_FOLDER_ID}, + 'folder_one_one – folder_one – Templates – service one – GOV.UK Notify', + 'Templates / folder_one / folder_one_one', + [ + {'template_type': 'all'}, + {'template_type': 'all', 'template_folder_id': PARENT_FOLDER_ID}, + ], {'template_folder_id': CHILD_FOLDER_ID}, ['Text message', 'Email', 'Letter'], [ @@ -279,9 +282,13 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare None, ), ( - 'folder_one_one_one – folder_one_one – service one – GOV.UK Notify', - 'folder_one_one / folder_one_one_one', - {'template_type': 'all', 'template_folder_id': CHILD_FOLDER_ID}, + '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', + [ + {'template_type': 'all'}, + {'template_type': 'all', 'template_folder_id': PARENT_FOLDER_ID}, + {'template_type': 'all', 'template_folder_id': CHILD_FOLDER_ID}, + ], {'template_folder_id': GRANDCHILD_FOLDER_ID}, ['Text message', 'Email', 'Letter'], [ @@ -298,7 +305,7 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare ( 'folder_two – Templates – service one – GOV.UK Notify', 'Templates / folder_two', - {'template_type': 'all'}, + [{'template_type': 'all'}], {'template_folder_id': FOLDER_TWO_ID}, ['Text message', 'Email', 'Letter'], [], @@ -309,7 +316,7 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare ( 'folder_two – Templates – service one – GOV.UK Notify', 'Templates / folder_two', - {'template_type': 'sms'}, + [{'template_type': 'sms'}], {'template_folder_id': FOLDER_TWO_ID, 'template_type': 'sms'}, ['All', 'Email', 'Letter'], [], @@ -368,15 +375,14 @@ def test_should_show_templates_folder_page( assert normalize_spaces(page.select_one('title').text) == expected_title_tag assert normalize_spaces(page.select_one('h1').text) == expected_page_title - if expected_parent_link_args: - assert len(page.select('h1 a')) == 1 - assert page.select_one('h1 a')['href'] == url_for( + assert len(page.select('h1 a')) == len(expected_parent_link_args) + + for index, parent_link in enumerate(page.select('h1 a')): + assert parent_link['href'] == url_for( 'main.choose_template', service_id=SERVICE_ONE_ID, - **expected_parent_link_args + **expected_parent_link_args[index] ) - else: - assert page.select_one('h1 a') is None links_in_page = page.select('.pill a') diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index b0d13ea32..cbb1930ab 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -267,7 +267,7 @@ def test_should_show_live_search_if_service_has_lots_of_folders( ) count_of_templates_and_folders = len(page.select('.message-name')) - count_of_folders = len(page.select('.template-list-folder')) + count_of_folders = len(page.select('.template-list-folder:first-child')) count_of_templates = count_of_templates_and_folders - count_of_folders assert len(page.select('.live-search')) == 1