From 9451531bc66be10486b37fe838674167d953afdd Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 18 Aug 2020 11:19:07 +0100 Subject: [PATCH 1/3] Fix first skip to contents link It was invisible at all times. Now it is visible when selected. This will help keyboard users take advantage of it. --- app/templates/views/check/ok.html | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/app/templates/views/check/ok.html b/app/templates/views/check/ok.html index 7554f18c0..a2fe13ab2 100644 --- a/app/templates/views/check/ok.html +++ b/app/templates/views/check/ok.html @@ -5,13 +5,9 @@ {% from "components/page-header.html" import page_header %} {% from "components/message-count-label.html" import message_count_label %} {% from "components/button/macro.njk" import govukButton %} +{% from "components/skip-link/macro.njk" import govukSkipLink %} {% set file_contents_header_id = 'file-preview' %} -{% macro skip_to_file_contents() %} -

- Skip to file contents -

-{% endmacro %} {% block service_page_title %} {{ "Preview of {}".format(template.name) }} @@ -30,7 +26,10 @@ {% endcall %} {% endif %} - {{ skip_to_file_contents() }} + {{ govukSkipLink({ + "text": "Skip to file contents", + "href": "#" + file_contents_header_id + }) }} {{ template|string }}
From 8659e46828ce60d3ffadc47577bfad3d7b07e6ee Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 18 Aug 2020 11:55:22 +0100 Subject: [PATCH 2/3] Remove skiplink & fix table caption on bulk send error pages The skiplink in those pages is in the error summary at the top. In this case, it seems redundant because: - it jumps to a section of the page just below it - the section it would jump to is the only error on the page Because of this, and because The Digital Accessibility Centre (DAC) reported that the skiplinks in these pages may not be needed, we decided to remove it. This commit also adds a prefix to the table caption to indicate that it only contains the errors from the CSV, not the rest of its contents. --- app/templates/views/check/column-errors.html | 11 +--- app/templates/views/check/row-errors.html | 10 +--- tests/app/main/views/test_send.py | 57 +++++++------------- 3 files changed, 21 insertions(+), 57 deletions(-) diff --git a/app/templates/views/check/column-errors.html b/app/templates/views/check/column-errors.html index b6c8e0e11..68bd99de1 100644 --- a/app/templates/views/check/column-errors.html +++ b/app/templates/views/check/column-errors.html @@ -6,13 +6,6 @@ {% from "components/back-link/macro.njk" import govukBackLink %} {% from "components/message-count-label.html" import message_count_label %} -{% set file_contents_header_id = 'file-preview' %} -{% macro skip_to_file_contents() %} -

- Skip to file contents -

-{% endmacro %} - {% block service_page_title %} Error {% endblock %} @@ -135,8 +128,6 @@ {% endif %} - {{ skip_to_file_contents() }} - {% endcall %}
@@ -158,7 +149,7 @@ {% set column_headers = recipients._raw_column_headers if recipients.duplicate_recipient_column_headers else recipients.column_headers %} -

{{ original_file_name }}

+

{{ original_file_name }}

{% call(item, row_number) list_table( diff --git a/app/templates/views/check/row-errors.html b/app/templates/views/check/row-errors.html index 4ffa028e4..b150c7704 100644 --- a/app/templates/views/check/row-errors.html +++ b/app/templates/views/check/row-errors.html @@ -6,13 +6,6 @@ {% from "components/back-link/macro.njk" import govukBackLink %} {% from "components/message-count-label.html" import message_count_label %} -{% set file_contents_header_id = 'file-preview' %} -{% macro skip_to_file_contents() %} -

- Skip to file contents -

-{% endmacro %} - {% block service_page_title %} Error {% endblock %} @@ -43,7 +36,6 @@ {% endfor %} {% endif %} - {{ skip_to_file_contents() }} {% endcall %}
@@ -60,7 +52,7 @@
{% call(item, row_number) mapping_table( - caption=original_file_name, + caption="Errors in " + original_file_name, caption_visible=False, field_headings=[ 'Row in file'|safe diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 1733b05d5..ba001d7e3 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -515,8 +515,7 @@ def test_upload_csv_file_with_empty_message_shows_check_page_with_errors( page.select_one('.banner-dangerous').text ) == ( 'There’s a problem with invalid.csv ' - 'You need to check you have content for the empty message in 1 row. ' - 'Skip to file contents' + 'You need to check you have content for the empty message in 1 row.' ) assert [ normalize_spaces(row.text) for row in page.select('tbody tr') @@ -571,8 +570,7 @@ def test_upload_csv_file_with_very_long_placeholder_shows_check_page_with_errors page.select_one('.banner-dangerous').text ) == ( 'There’s a problem with invalid.csv ' - 'You need to shorten the messages in 2 rows. ' - 'Skip to file contents' + 'You need to shorten the messages in 2 rows.' ) assert [ normalize_spaces(row.text) for row in page.select('tbody tr') @@ -631,8 +629,7 @@ def test_upload_csv_file_with_bad_postal_address_shows_check_page_with_errors( page.select_one('.banner-dangerous').text ) == ( 'There’s a problem with invalid.csv ' - 'You need to fix 5 addresses. ' - 'Skip to file contents' + 'You need to fix 5 addresses.' ) assert [ normalize_spaces(row.text) for row in page.select('tbody tr') @@ -692,8 +689,7 @@ def test_upload_csv_file_with_international_letters_permission_shows_appropriate page.select_one('.banner-dangerous').text ) == ( 'There’s a problem with invalid.csv ' - 'You need to fix 2 addresses. ' - 'Skip to file contents' + 'You need to fix 2 addresses.' ) assert [ normalize_spaces(row.text) for row in page.select('tbody tr') @@ -715,8 +711,7 @@ def test_upload_csv_file_with_international_letters_permission_shows_appropriate ( 'There’s a problem with your column names ' 'Your file needs a column called ‘phone number’. ' - 'Right now it has columns called ‘telephone’ and ‘name’. ' - 'Skip to file contents' + 'Right now it has columns called ‘telephone’ and ‘name’.' ) ), ( @@ -726,8 +721,7 @@ def test_upload_csv_file_with_international_letters_permission_shows_appropriate """, ( 'Your column names need to match the double brackets in your template ' - 'Your file is missing a column called ‘name’. ' - 'Skip to file contents' + 'Your file is missing a column called ‘name’.' ) ), ( @@ -738,8 +732,7 @@ def test_upload_csv_file_with_international_letters_permission_shows_appropriate ( 'There’s a problem with your column names ' 'We found more than one column called ‘phone number’ or ‘PHONE_NUMBER’. ' - 'Delete or rename one of these columns and try again. ' - 'Skip to file contents' + 'Delete or rename one of these columns and try again.' ) ), ( @@ -748,24 +741,21 @@ def test_upload_csv_file_with_international_letters_permission_shows_appropriate """, ( 'Your file is missing some rows ' - 'It needs at least one row of data. ' - 'Skip to file contents' + 'It needs at least one row of data.' ) ), ( "+447700900986", ( 'Your file is missing some rows ' - 'It needs at least one row of data, and columns called ‘name’ and ‘phone number’. ' - 'Skip to file contents' + 'It needs at least one row of data, and columns called ‘name’ and ‘phone number’.' ) ), ( "", ( 'Your file is missing some rows ' - 'It needs at least one row of data, and columns called ‘name’ and ‘phone number’. ' - 'Skip to file contents' + 'It needs at least one row of data, and columns called ‘name’ and ‘phone number’.' ) ), ( @@ -777,8 +767,7 @@ def test_upload_csv_file_with_international_letters_permission_shows_appropriate """, ( 'There’s a problem with invalid.csv ' - 'You need to enter missing data in 1 row. ' - 'Skip to file contents' + 'You need to enter missing data in 1 row.' ) ), ( @@ -790,8 +779,7 @@ def test_upload_csv_file_with_international_letters_permission_shows_appropriate """, ( 'There’s a problem with invalid.csv ' - 'You need to enter missing data in 1 row. ' - 'Skip to file contents' + 'You need to enter missing data in 1 row.' ) ), ]) @@ -3343,8 +3331,7 @@ def test_check_messages_shows_trial_mode_error( page.find('div', class_='banner-dangerous').text.split() ) == ( 'You cannot send to this phone number ' - 'In trial mode you can only send to yourself and members of your team ' - 'Skip to file contents' + 'In trial mode you can only send to yourself and members of your team' ) @@ -3406,8 +3393,7 @@ def test_check_messages_shows_trial_mode_error_for_letters( if error_should_be_shown: assert normalize_spaces(error[0].text) == ( '{} ' - 'In trial mode you can only preview how your letters will look ' - 'Skip to file contents' + 'In trial mode you can only preview how your letters will look' ).format(expected_error_message) else: assert not error @@ -3509,8 +3495,7 @@ def test_check_messages_shows_data_errors_before_trial_mode_errors_for_letters( assert normalize_spaces(page.select_one('.banner-dangerous').text) == ( 'There’s a problem with example.xlsx ' - 'You need to fix 2 addresses. ' - 'Skip to file contents' + 'You need to fix 2 addresses.' ) assert not page.select('.table-field-index a') @@ -3551,8 +3536,7 @@ def test_warns_if_file_sent_already( page.select_one('.banner-dangerous').text ) == ( 'These messages have already been sent today ' - 'If you need to resend them, rename the file and upload it again. ' - 'Skip to file contents' + 'If you need to resend them, rename the file and upload it again.' ) mock_get_jobs.assert_called_once_with(SERVICE_ONE_ID, limit_days=0) @@ -3600,8 +3584,7 @@ def test_check_messages_column_error_doesnt_show_optional_columns( 'There’s a problem with your column names ' 'Your file needs at least 3 address columns, for example ‘address line 1’, ' '‘address line 2’ and ‘address line 3’. ' - 'Right now it has columns called ‘address_line_1’, ‘address_line_2’ and ‘foo’. ' - 'Skip to file contents' + 'Right now it has columns called ‘address_line_1’, ‘address_line_2’ and ‘foo’.' ) @@ -3697,8 +3680,7 @@ def test_letters_from_csv_files_dont_have_download_link( page.select_one('.banner-dangerous').text ) == normalize_spaces( 'You cannot send this letter ' - 'In trial mode you can only preview how your letters will look ' - 'Skip to file contents' + 'In trial mode you can only preview how your letters will look' ) assert len(page.select('.letter img')) == 5 @@ -3873,8 +3855,7 @@ def test_check_messages_shows_over_max_row_error( ) == ( 'Your file has too many rows ' 'Notify can process up to 11,111 rows at once. ' - 'Your file has 99,999 rows. ' - 'Skip to file contents' + 'Your file has 99,999 rows.' ) From 33ebd01d70c40c56ad3bfbaa23691195e254ab01 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 18 Aug 2020 12:25:47 +0100 Subject: [PATCH 3/3] Remove unused skip-link from contact list upload pages Since file contents are directly below the header, there is no need for a skip link moving focus to those file contents. --- .../uploads/contact-list/column-errors.html | 12 +----- .../views/uploads/contact-list/ok.html | 7 ---- .../uploads/contact-list/row-errors.html | 7 ---- .../contact-list/too-many-columns.html | 11 +----- .../views/uploads/test_upload_contact_list.py | 39 +++++++------------ 5 files changed, 15 insertions(+), 61 deletions(-) diff --git a/app/templates/views/uploads/contact-list/column-errors.html b/app/templates/views/uploads/contact-list/column-errors.html index 99f5354a1..96a7becb1 100644 --- a/app/templates/views/uploads/contact-list/column-errors.html +++ b/app/templates/views/uploads/contact-list/column-errors.html @@ -6,13 +6,6 @@ {% from "components/back-link/macro.njk" import govukBackLink %} {% from "components/message-count-label.html" import message_count_label, recipient_count_label %} -{% set file_contents_header_id = 'file-preview' %} -{% macro skip_to_file_contents() %} -

- Skip to file contents -

-{% endmacro %} - {% block service_page_title %} Error {% endblock %} @@ -48,9 +41,6 @@

{% endif %} - - {{ skip_to_file_contents() }} - {% endcall %}
@@ -66,7 +56,7 @@ Back to top -

{{ original_file_name }}

+

{{ original_file_name }}

{% call(item, row_number) list_table( recipients.displayed_rows, diff --git a/app/templates/views/uploads/contact-list/ok.html b/app/templates/views/uploads/contact-list/ok.html index 0dfa68a4a..1db87a58a 100644 --- a/app/templates/views/uploads/contact-list/ok.html +++ b/app/templates/views/uploads/contact-list/ok.html @@ -6,13 +6,6 @@ {% from "components/message-count-label.html" import message_count_label, recipient_count_label %} {% from "components/button/macro.njk" import govukButton %} -{% set file_contents_header_id = 'file-preview' %} -{% macro skip_to_file_contents() %} -

- Skip to file contents -

-{% endmacro %} - {% block service_page_title %} {{ original_file_name }} {% endblock %} diff --git a/app/templates/views/uploads/contact-list/row-errors.html b/app/templates/views/uploads/contact-list/row-errors.html index 6480e00c8..97e496568 100644 --- a/app/templates/views/uploads/contact-list/row-errors.html +++ b/app/templates/views/uploads/contact-list/row-errors.html @@ -6,12 +6,6 @@ {% from "components/back-link/macro.njk" import govukBackLink %} {% from "components/message-count-label.html" import message_count_label %} -{% set file_contents_header_id = 'file-preview' %} -{% macro skip_to_file_contents() %} -

- Skip to file contents -

-{% endmacro %} {% block service_page_title %} Error @@ -43,7 +37,6 @@ {% endfor %} {% endif %} - {{ skip_to_file_contents() }} {% endcall %} diff --git a/app/templates/views/uploads/contact-list/too-many-columns.html b/app/templates/views/uploads/contact-list/too-many-columns.html index c9f64e2a7..83ed244e6 100644 --- a/app/templates/views/uploads/contact-list/too-many-columns.html +++ b/app/templates/views/uploads/contact-list/too-many-columns.html @@ -6,12 +6,6 @@ {% from "components/back-link/macro.njk" import govukBackLink %} {% from "components/message-count-label.html" import message_count_label %} -{% set file_contents_header_id = 'file-preview' %} -{% macro skip_to_file_contents() %} -

- Skip to file contents -

-{% endmacro %} {% block service_page_title %} Error @@ -56,9 +50,6 @@

{% endif %} - - {{ skip_to_file_contents() }} - {% endcall %} @@ -76,7 +67,7 @@ {% set column_headers = recipients._raw_column_headers if recipients.duplicate_recipient_column_headers else recipients.column_headers %} -

{{ original_file_name }}

+

{{ original_file_name }}

{% call(item, row_number) list_table( diff --git a/tests/app/main/views/uploads/test_upload_contact_list.py b/tests/app/main/views/uploads/test_upload_contact_list.py index af2daf85d..8e246788f 100644 --- a/tests/app/main/views/uploads/test_upload_contact_list.py +++ b/tests/app/main/views/uploads/test_upload_contact_list.py @@ -40,8 +40,7 @@ def test_upload_contact_list_page(client_request): ( 'Your file has too many columns ' 'It needs to have 1 column, called ‘email address’ or ‘phone number’. ' - 'Right now it has 2 columns called ‘telephone’ and ‘name’. ' - 'Skip to file contents' + 'Right now it has 2 columns called ‘telephone’ and ‘name’.' ), 'Row in file 1 telephone name', '2 +447700900986', @@ -54,8 +53,7 @@ def test_upload_contact_list_page(client_request): ( 'Your file has too many columns ' 'It needs to have 1 column, called ‘email address’ or ‘phone number’. ' - 'Right now it has 2 columns called ‘phone number’ and ‘email address’. ' - 'Skip to file contents' + 'Right now it has 2 columns called ‘phone number’ and ‘email address’.' ), 'Row in file 1 phone number email address', '2 +447700900986 test@example.com', @@ -67,8 +65,7 @@ def test_upload_contact_list_page(client_request): """, ( 'There’s a problem with invalid.csv ' - 'You need to fix 1 email address. ' - 'Skip to file contents' + 'You need to fix 1 email address.' ), 'Row in file 1 email address', '2 Not a valid email address +447700900986', @@ -80,8 +77,7 @@ def test_upload_contact_list_page(client_request): """, ( 'There’s a problem with invalid.csv ' - 'You need to fix 1 phone number. ' - 'Skip to file contents' + 'You need to fix 1 phone number.' ), 'Row in file 1 phone number', '2 Must not contain letters or symbols test@example.com', @@ -94,8 +90,7 @@ def test_upload_contact_list_page(client_request): ( 'Your file has too many columns ' 'It needs to have 1 column, called ‘email address’ or ‘phone number’. ' - 'Right now it has 3 columns called ‘phone number’, ‘phone number’ and ‘PHONE_NUMBER’. ' - 'Skip to file contents' + 'Right now it has 3 columns called ‘phone number’, ‘phone number’ and ‘PHONE_NUMBER’.' ), 'Row in file 1 phone number phone number PHONE_NUMBER', '2 +447700900333 +447700900333 +447700900333', @@ -106,8 +101,7 @@ def test_upload_contact_list_page(client_request): """, ( 'Your file is missing some rows ' - 'It needs at least one row of data. ' - 'Skip to file contents' + 'It needs at least one row of data.' ), 'Row in file 1 phone number', '', @@ -117,8 +111,7 @@ def test_upload_contact_list_page(client_request): ( 'Your file is missing some rows ' 'It needs at least one row of data, in a column called ' - '‘email address’ or ‘phone number’. ' - 'Skip to file contents' + '‘email address’ or ‘phone number’.' ), 'Row in file 1 +447700900986', '', @@ -128,8 +121,7 @@ def test_upload_contact_list_page(client_request): ( 'Your file is missing some rows ' 'It needs at least one row of data, in a column called ' - '‘email address’ or ‘phone number’. ' - 'Skip to file contents' + '‘email address’ or ‘phone number’.' ), 'Row in file 1', '', @@ -143,8 +135,7 @@ def test_upload_contact_list_page(client_request): """, ( 'There’s a problem with invalid.csv ' - 'You need to enter missing data in 1 row. ' - 'Skip to file contents' + 'You need to enter missing data in 1 row.' ), 'Row in file 1 phone number', ( @@ -158,8 +149,7 @@ def test_upload_contact_list_page(client_request): """, ( 'There’s a problem with invalid.csv ' - 'You need to fix 1 phone number. ' - 'Skip to file contents' + 'You need to fix 1 phone number.' ), 'Row in file 1 phone number', '2 Not enough digits +447700900', @@ -173,8 +163,7 @@ def test_upload_contact_list_page(client_request): """, ( 'There’s a problem with invalid.csv ' - 'You need to fix 2 email addresses. ' - 'Skip to file contents' + 'You need to fix 2 email addresses.' ), 'Row in file 1 email address', ( @@ -257,8 +246,7 @@ def test_upload_csv_file_shows_error_banner_for_too_many_rows( assert normalize_spaces(page.select_one('.banner-dangerous').text) == ( 'Your file has too many rows ' 'Notify can store files up to 50,000 rows in size. ' - 'Your file has 50,001 rows. ' - 'Skip to file contents' + 'Your file has 50,001 rows.' ) assert len(page.select('tbody tr')) == 50 assert normalize_spaces(page.select_one('.table-show-more-link').text) == ( @@ -288,8 +276,7 @@ def test_upload_csv_shows_trial_mode_error( assert normalize_spaces(page.select_one('.banner-dangerous').text) == ( 'You cannot save this phone number ' - 'In trial mode you can only send to yourself and members of your team ' - 'Skip to file contents' + 'In trial mode you can only send to yourself and members of your team' ) assert page.select_one('.banner-dangerous a')['href'] == url_for( 'main.trial_mode_new'