From fe1a306285da6c48b66506c589abc2c66b51668a Mon Sep 17 00:00:00 2001 From: DevilsAutumn Date: Sat, 9 Dec 2023 16:20:57 +0530 Subject: [PATCH] Move to a select field for determining the way TableBlock will use headers - Make the new table_header_choice select backwards compatible with tables stored before Wagtail 6.0 - Built on previous PRs #9673 & #6763 - Fixes #5989 --- CHANGELOG.txt | 1 + .../__snapshots__/table.test.js.snap | 74 +++++++++---------- .../entrypoints/contrib/table_block/table.js | 70 ++++++++---------- .../contrib/table_block/table.test.js | 30 +++++--- docs/releases/6.0.md | 1 + wagtail/contrib/table_block/blocks.py | 55 +++++++++++++- wagtail/contrib/table_block/tests.py | 59 ++++++++++++++- 7 files changed, 194 insertions(+), 96 deletions(-) diff --git a/CHANGELOG.txt b/CHANGELOG.txt index b4286d6323..ea5bf83971 100644 --- a/CHANGELOG.txt +++ b/CHANGELOG.txt @@ -38,6 +38,7 @@ Changelog * Move locale selector in generic IndexView to a filter (Sage Abdullah) * Add ability to customise a page's copy form (Neeraj Yetheendran) * Add optional caption field to `TypedTableBlock` (Tommaso Amici, Cynthia Kiser) + * Switch the `TableBlock` header controls to a field that requires user input (Bhuvnesh Sharma, Aman Pandey, Cynthia Kiser) * Fix: Update system check for overwriting storage backends to recognise the `STORAGES` setting introduced in Django 4.2 (phijma-leukeleu) * Fix: Prevent password change form from raising a validation error when browser autocomplete fills in the "Old password" field (Chiemezuo Akujobi) * Fix: Ensure that the legacy dropdown options, when closed, do not get accidentally clicked by other interactions wide viewports (CheesyPhoenix, Christer Jensen) diff --git a/client/src/entrypoints/contrib/table_block/__snapshots__/table.test.js.snap b/client/src/entrypoints/contrib/table_block/__snapshots__/table.test.js.snap index fe6ebd08db..067b667b44 100644 --- a/client/src/entrypoints/contrib/table_block/__snapshots__/table.test.js.snap +++ b/client/src/entrypoints/contrib/table_block/__snapshots__/table.test.js.snap @@ -3,26 +3,23 @@ exports[`telepath: wagtail.widgets.TableInput it renders correctly 1`] = ` "
- -
-
-
Display the first row as a header.
-
-
- -
-
-
-
- -
-
-
Display the first column as a header.
-
-
- -
-
+ + +

Which cells should be displayed as headers?

@@ -43,26 +40,23 @@ exports[`telepath: wagtail.widgets.TableInput it renders correctly 1`] = ` exports[`telepath: wagtail.widgets.TableInput translation 1`] = ` "
- -
-
-
Affichez la première ligne sous forme d'en-tête.
-
-
- -
-
-
-
- -
-
-
Affichez la première colonne sous forme d'en-tête.
-
-
- -
-
+ + +

Quelles cellules doivent être affichées en tant qu'en-têtes?

diff --git a/client/src/entrypoints/contrib/table_block/table.js b/client/src/entrypoints/contrib/table_block/table.js index 134b7edd40..a0292070d3 100644 --- a/client/src/entrypoints/contrib/table_block/table.js +++ b/client/src/entrypoints/contrib/table_block/table.js @@ -7,12 +7,14 @@ import { hasOwn } from '../../../utils/hasOwn'; function initTable(id, tableOptions) { const containerId = id + '-handsontable-container'; - const tableHeaderCheckboxId = id + '-handsontable-header'; - const colHeaderCheckboxId = id + '-handsontable-col-header'; + var tableHeaderId = id + '-handsontable-header'; + var colHeaderId = id + '-handsontable-col-header'; + var headerChoiceId = id + '-table-header-choice'; const tableCaptionId = id + '-handsontable-col-caption'; const hiddenStreamInput = $('#' + id); - const tableHeaderCheckbox = $('#' + tableHeaderCheckboxId); - const colHeaderCheckbox = $('#' + colHeaderCheckboxId); + var tableHeader = $('#' + tableHeaderId); + var colHeader = $('#' + colHeaderId); + var headerChoice = $('#' + headerChoiceId); const tableCaption = $('#' + tableCaptionId); const finalOptions = {}; let hot = null; @@ -52,18 +54,12 @@ function initTable(id, tableOptions) { } if (dataForForm !== null) { - if (hasOwn(dataForForm, 'first_row_is_table_header')) { - tableHeaderCheckbox.prop( - 'checked', - dataForForm.first_row_is_table_header, - ); - } - if (hasOwn(dataForForm, 'first_col_is_header')) { - colHeaderCheckbox.prop('checked', dataForForm.first_col_is_header); - } if (hasOwn(dataForForm, 'table_caption')) { tableCaption.prop('value', dataForForm.table_caption); } + if (hasOwn(dataForForm, 'table_header_choice')) { + headerChoice.prop('value', dataForForm.table_header_choice); + } } if (!hasOwn(tableOptions, 'width') || !hasOwn(tableOptions, 'height')) { @@ -123,8 +119,9 @@ function initTable(id, tableOptions) { data: hot.getData(), cell: cell, mergeCells: mergeCells, - first_row_is_table_header: tableHeaderCheckbox.prop('checked'), - first_col_is_header: colHeaderCheckbox.prop('checked'), + first_row_is_table_header: tableHeader.val(), + first_col_is_header: colHeader.val(), + table_header_choice: headerChoice.val(), table_caption: tableCaption.val(), }), ); @@ -169,11 +166,7 @@ function initTable(id, tableOptions) { persist(); }; - tableHeaderCheckbox.on('change', () => { - persist(); - }); - - colHeaderCheckbox.on('change', () => { + headerChoice.on('change', () => { persist(); }); @@ -238,26 +231,23 @@ class TableInput { const container = document.createElement('div'); container.innerHTML = `
- -
-
-
${this.strings['Display the first row as a header.']}
-
-
- -
-
-
-
- -
-
-
${this.strings['Display the first column as a header.']}
-
-
- -
-
+ + +

${this.strings['Which cells should be displayed as headers?']}

diff --git a/client/src/entrypoints/contrib/table_block/table.test.js b/client/src/entrypoints/contrib/table_block/table.test.js index 26feabba33..462f7f35f2 100644 --- a/client/src/entrypoints/contrib/table_block/table.test.js +++ b/client/src/entrypoints/contrib/table_block/table.test.js @@ -33,11 +33,15 @@ const TEST_OPTIONS = { }; const TEST_STRINGS = { - 'Row header': 'Row header', - 'Display the first row as a header.': 'Display the first row as a header.', - 'Column header': 'Column header', - 'Display the first column as a header.': - 'Display the first column as a header.', + 'Table headers': 'Table headers', + 'Display the first row as a header': 'Display the first row as a header', + 'Display the first column as a header': + 'Display the first column as a header', + 'Display the first row AND first column as headers': + 'Display the first row AND first column as headers', + 'No headers': 'No headers', + 'Which cells should be displayed as headers?': + 'Which cells should be displayed as headers?', 'Table caption': 'Table caption', 'A heading that identifies the overall topic of the table, and is useful for screen reader users.': @@ -155,12 +159,16 @@ describe('telepath: wagtail.widgets.TableInput', () => { test('translation', () => { testStrings = { - 'Row header': 'En-tête de ligne', - 'Display the first row as a header.': - "Affichez la première ligne sous forme d'en-tête.", - 'Column header': 'En-tête de colonne', - 'Display the first column as a header.': - "Affichez la première colonne sous forme d'en-tête.", + 'Table headers': 'En-têtes de tableau', + 'Display the first row as a header': + "Afficher la première ligne sous forme d'en-tête", + 'Display the first column as a header': + "Afficher la première colonne sous forme d'en-tête", + 'Display the first row AND first column as headers': + "Afficher la première ligne ET la première colonne sous forme d'en-têtes", + 'No headers': "Pas d'en-têtes", + 'Which cells should be displayed as headers?': + "Quelles cellules doivent être affichées en tant qu'en-têtes?", 'Table caption': 'Légende du tableau', 'A heading that identifies the overall topic of the table, and is useful for screen reader users.': diff --git a/docs/releases/6.0.md b/docs/releases/6.0.md index 2016741296..4b004d7b38 100644 --- a/docs/releases/6.0.md +++ b/docs/releases/6.0.md @@ -60,6 +60,7 @@ Thank you to Thibaud Colas and Badr Fourane for their work on this feature. * Show character counts on RichTextBlock with `max_length` (Elhussein Almasri) * Move locale selector in generic IndexView to a filter (Sage Abdullah) * Add optional caption field to `TypedTableBlock` (Tommaso Amici, Cynthia Kiser) + * Switch the `TableBlock` header controls to a field that requires user input (Bhuvnesh Sharma, Aman Pandey, Cynthia Kiser) ### Bug fixes diff --git a/wagtail/contrib/table_block/blocks.py b/wagtail/contrib/table_block/blocks.py index dcacd9b4f7..6e5e420013 100644 --- a/wagtail/contrib/table_block/blocks.py +++ b/wagtail/contrib/table_block/blocks.py @@ -1,6 +1,9 @@ import json from django import forms +from django.core.exceptions import ValidationError +from django.forms.fields import Field +from django.forms.utils import ErrorList from django.template.loader import render_to_string from django.utils import translation from django.utils.functional import cached_property @@ -68,12 +71,18 @@ class TableInputAdapter(WidgetAdapter): def js_args(self, widget): strings = { "Row header": _("Row header"), - "Display the first row as a header.": _( - "Display the first row as a header." + "Table headers": _("Table headers"), + "Display the first row as a header": _("Display the first row as a header"), + "Display the first column as a header": _( + "Display the first column as a header" ), "Column header": _("Column header"), - "Display the first column as a header.": _( - "Display the first column as a header." + "Display the first row AND first column as headers": _( + "Display the first row AND first column as headers" + ), + "No headers": _("No headers"), + "Which cells should be displayed as headers?": _( + "Which cells should be displayed as headers?" ), "Table caption": _("Table caption"), "A heading that identifies the overall topic of the table, and is useful for screen reader users.": _( @@ -117,6 +126,44 @@ class TableBlock(FieldBlock): def value_for_form(self, value): return json.dumps(value) + def to_python(self, value): + """ + If value came from a table block stored before Wagtail 6.0, we need to set an appropriate + value for the header choice. I would really like to have this default to "" and force the + editor to reaffirm they don't want any headers, but that woud be a breaking change. + """ + if not value.get("table_header_choice", ""): + if value.get("first_row_is_table_header", False) and value.get( + "first_col_is_header", False + ): + value["table_header_choice"] = "both" + elif value.get("first_row_is_table_header", False): + value["table_header_choice"] = "row" + elif value.get("first_col_is_header", False): + value["table_header_choice"] = "col" + else: + value["table_header_choice"] = "neither" + return value + + def clean(self, value): + if not value: + return value + + if value.get("table_header_choice", ""): + value["first_row_is_table_header"] = value["table_header_choice"] in [ + "row", + "both", + ] + value["first_col_is_header"] = value["table_header_choice"] in [ + "column", + "both", + ] + else: + # Ensure we have a choice for the table_header_choice + errors = ErrorList(Field.default_error_messages["required"]) + raise ValidationError("Validation error in TableBlock", params=errors) + return self.value_from_form(self.field.clean(self.value_for_form(value))) + def get_form_state(self, value): # pass state to frontend as a JSON-ish dict - do not serialise to a JSON string return value diff --git a/wagtail/contrib/table_block/tests.py b/wagtail/contrib/table_block/tests.py index 085dbe8f41..51e9eff6dd 100644 --- a/wagtail/contrib/table_block/tests.py +++ b/wagtail/contrib/table_block/tests.py @@ -116,7 +116,7 @@ class TestTableBlock(TestCase): """ self.assertHTMLEqual(result, expected) - def test_do_not_render_html(self): + def test_do_not_render_html_by_default(self): """ Ensure that raw html doesn't render by default. @@ -145,6 +145,36 @@ class TestTableBlock(TestCase): result = block.render(value) self.assertHTMLEqual(result, expected) + def test_does_render_html_if_allowed(self): + """ + Ensure html renders if table_options set renderer to allow html + """ + value = { + "first_row_is_table_header": False, + "first_col_is_header": False, + "data": [ + ["

Test

", None, None], + [None, None, None], + [None, None, None], + ], + } + + expected = """ + + + + + + +

Test

+ """ + + new_options = self.default_table_options.copy() + new_options["renderer"] = "html" + block = TableBlock(table_options=new_options) + result = block.render(value) + self.assertHTMLEqual(result, expected) + def test_row_headers(self): """ Ensure that row headers are properly rendered. @@ -226,6 +256,33 @@ class TestTableBlock(TestCase): result = block.render(value) self.assertHTMLEqual(result, expected) + def test_no_headers(self): + """ + Test table without headers. + """ + value = { + "first_row_is_table_header": False, + "first_col_is_header": False, + "data": [ + ["Foo", "Bar", "Baz"], + ["one", "two", "three"], + ["four", "five", "six"], + ], + } + + expected = """ + + + + + + +
FooBarBaz
onetwothree
fourfivesix
+ """ + block = TableBlock() + result = block.render(value) + self.assertHTMLEqual(result, expected) + def test_value_for_and_from_form(self): """ Make sure we get back good json and make