Trim titles in basic settings (#9598) #9599

andrewnicols · 2024-04-29T15:41:57Z

The buildBasicSettingsDataset call is used for:

block_formats
font_family_formats
font_size_formats

The documented values for both block_formats and font_family_formats include a space between the ; and the title, for example the default value in block_formats is:

Paragraph=p; Heading 1=h1; Heading 2=h2; Heading 3=h3; Heading 4=h4; Heading 5=h5; Heading 6=h6;

However the split() method in SelectDatasets.ts splits on the delimter, leading to values such as Heading 1=h1 which are then passed into the process() method which just split them on = leading to a title of Heading 1 (with leading whitespace) which does not match any valid translation.

This patch modifies the split() method to trim the title component.

Happy to add tests if you can tell me where best to add them. I had a look but couldn't find unit tests for any of this functionality.

Related Ticket:

Description of Changes:

Placeholder text

Pre-checks:

Changelog entry added
Tests have been added (if applicable)
Branch prefixed with feature/, hotfix/ or spike/

Review:

Milestone set
Docs ticket created (if applicable)

GitHub issues (if applicable):
#9598

andrewnicols · 2024-04-29T15:43:51Z

Note: I considered whether the docs should be fixed instead, but there are existing configurations out there which are broken if the language is changed.

TheSpyder · 2024-04-30T02:33:53Z

Thank you for the PR! Looks like the best test we have is one for font_family_formats, which coincidentally had to trim the string before comparing it to the expected. Your fix should make that trim unnecessary?

tinymce/modules/tinymce/src/core/test/ts/browser/FontSelectCustomTest.ts

Line 25 in c47600f

const value = Strings.trim(TextContent.get(selectBox) ?? '');

Ideally we'd have a smaller test for SelectDatasets in modules/tinymce/src/themes/silver/test/ts/atomic somewhere but that would likely require refactoring SelectDatasets so the test doesn't need a full editor just to get the option value to process.

TheSpyder · 2024-04-30T02:45:48Z

For the changelog requirement, we use changie - you don't need it installed, just run yarn changie new and select tinymce as the project. It will ask for a TINY- code, that's from our internal JIRA, just use the github issue number and replace the TINY- prefix in the file it creates with GH- before committing.

EkimChau · 2024-04-30T02:54:42Z

Hello andrewnicols,

Thank you for your code contribution, in order for Tiny Inc to accept your code, we will need permission to use your code contribution, this can be done through the Tiny Contributor License Agreement (Tiny CLA). Please contact legal@tiny.cloud to sign the Tiny CLA agreement.

andrewnicols

Thanks folks,

@EkimChau I have pinged legal for information on signing the CLA

@TheSpyder:

I've created the changelog entry with changie
I've removed the existing Trim from modules/tinymce/src/core/test/ts/browser/FontSelectCustomTest.ts and confirmed that the test fails before this change, and passes afterwards.

I've also pulled out the relevant parts of SelectDatasets.ts into a new ./utils/Select.ts location whcih seemed to be the most appropriate location, and written unit tests for this.

In doing so I made some additional changes to better handle invalid configuration, including:

spaces before around the value; and multiple spaces in a space-delimited string.

TypeScript isn't my daily langauge so I do have a question regarding the use of the Delimiters.SemiColon. In the unit test I wanted to use the literal semicolon (;), but tsc complains because it doens't match the Delimiters. What should I do here?

Thanks,

Andrew

andrewnicols · 2024-04-30T05:03:22Z

Oh, and I've also switched from title.trim() to using Strings.trim(title) which seems to be more in-keeping with the rest of the codebase.

The `buildBasicSettingsDataset` call is used for: - `block_formats` - `font_family_formats` - `font_size_formats` The documented values for both `block_formats` and `font_family_formats` include a space between the `;` and the title, for example the default value in `block_formats` is: ``` Paragraph=p; Heading 1=h1; Heading 2=h2; Heading 3=h3; Heading 4=h4; Heading 5=h5; Heading 6=h6; ``` However the `split()` method in `SelectDatasets.ts` splits on the delimiter, leading to values such as ` Heading 1=h1` which are then passed into the `process()` method which just split them on `=` leading to a title of ` Heading 1` (with leading whitespace) which does not match any valid translation. This patch modifies the `split()` method to trim the title component. It also refactors the SelectDatasets module to move the configuration processing into a utility method, and allows it to be unit tested. Additionally any white-spare around the `=` or `;` leads to erroneous values, for example a value of: ``` Paragraph = p ; ``` Leads to: title: `Paragraph ` value: ` p ` And multiple contiguous spaces leads to additional values, for example: ``` 12pt 14pt 16pt 18pt ``` Leads to a set of values: - `12pt` - `` (empty value) - `14pt` - `16pt` - `18pt`

Ensures that the values are subsequently translated correctly. Workaround for upstream bug: tinymce/tinymce#9599 Co-authored-by: Andrew Nicols <andrew@nicols.co.uk>

TheSpyder · 2024-05-03T05:52:51Z

TypeScript isn't my daily langauge so I do have a question regarding the use of the Delimiters.SemiColon. In the unit test I wanted to use the literal semicolon (;), but tsc complains because it doens't match the Delimiters. What should I do here?

You can't use literal strings, it's designed that way. You must reference the enum directly - which it looks like you did.

TheSpyder

Looking pretty good. Please make sure you run yarn lint at some point - I can see what looks like one or two lint errors (we don't have public CI so that won't run until we merge it).

TheSpyder · 2024-05-03T05:57:39Z

.changes/unreleased/tinymce-GH-9599-2024-04-30.yaml

@@ -0,0 +1,6 @@
+project: tinymce
+kind: Fixed
+body: Select Dataset content should be trimmed and filtered before parsing


Suggested change

body: Select Dataset content should be trimmed and filtered before parsing

body: `block_formats` and `font_family_formats` configuration parsing did not trim spaces, resulting in untranslated strings. Patch contributed by andrewnicols

TheSpyder · 2024-05-03T06:34:25Z

modules/tinymce/src/themes/silver/main/ts/ui/core/complex/SelectDatasets.ts

+  Delimiter,
+  BasicSelectItem


Since the refactored code is in the core, not moved to tests, you can just change where code that imports Delimiter and BasicSelectItem to utils/Select rather than re-export it here? There doesn't look like too many of them.

Alternatively, we do have places that export methods from modules only for testing (with a comment to that effect). So you could undo the refactoring if you prefer.

TheSpyder · 2024-05-03T06:43:22Z

modules/tinymce/src/themes/silver/main/ts/ui/core/complex/utils/Select.ts

+const process = (rawFormats: string[]): BasicSelectItem[] => Arr.map(rawFormats, (item) => {
+  let title = item, format = item;
+  // Allow text=value block formats
+  const values = Arr.filter(item.split('='), (value) => Strings.isNotEmpty(value));


Nicely done - but that's not quite all the edge cases. We do use Strings most of the time, but I think what would be best here is to filter out strings that are empty or contain only whitespace. Maybe someone makes a typo like Heading = = h1; (that's probably a good test to add, actually).

Code to check whitespace is in a different module for.... reasons.

tinymce/modules/tinymce/src/core/main/ts/text/Whitespace.ts

Line 5 in b4c1a77

const isWhitespaceText = (text: string): boolean => whiteSpaceRegExp.test(text);

TheSpyder · 2024-05-03T06:44:22Z

modules/tinymce/src/themes/silver/main/ts/ui/core/complex/utils/Select.ts

+
+const split = (rawFormats: string, delimiter: Delimiter): string[] => {
+  if (delimiter === Delimiter.SemiColon) {
+    return rawFormats.replace(/;$/, '').split(';');


With your new filter to handle empty strings, I suspect we don't need to remove the trailing ; anymore 🤔

Suggested change

return rawFormats.replace(/;$/, '').split(';');

return rawFormats.split(';');

TheSpyder · 2024-05-03T06:47:32Z

modules/tinymce/src/themes/silver/test/ts/atomic/ui/core/complex/utils/SelectTest.ts

+      const config = 'Paragraph=p; Heading 1=h1;Heading 2=h2; Heading 3 =h3;Pre =pre';
+      const items = makeBasicSelectItems(config, Delimiter.SemiColon);
+      assert.isArray(items);
+      assert.lengthOf(items, 5);


Length doesn't need to be checked if you're doing a deepEqual - the length will be part of that check already.

It should also print a much nicer error message than "expected length of 5 was 6" or whatever

andrewnicols requested a review from a team as a code owner April 29, 2024 15:41

andrewnicols requested review from spocke, TheSpyder and hamza0867 April 29, 2024 15:41

TheSpyder added the Community PR label Apr 30, 2024

TheSpyder added this to the 7.2.0 milestone Apr 30, 2024

andrewnicols force-pushed the spaceBeforeTitleBlockFormats branch 2 times, most recently from 324214f to da9061a Compare April 30, 2024 04:58

andrewnicols commented Apr 30, 2024

View reviewed changes

andrewnicols force-pushed the spaceBeforeTitleBlockFormats branch from da9061a to e19be5e Compare April 30, 2024 05:06

TheSpyder reviewed May 3, 2024

View reviewed changes

TheSpyder linked an issue May 3, 2024 that may be closed by this pull request

block_formats options not translated if using documented approach #9598

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trim titles in basic settings (#9598) #9599

Trim titles in basic settings (#9598) #9599

andrewnicols commented Apr 29, 2024 •

edited

andrewnicols commented Apr 29, 2024

TheSpyder commented Apr 30, 2024

TheSpyder commented Apr 30, 2024

EkimChau commented Apr 30, 2024

andrewnicols left a comment

andrewnicols commented Apr 30, 2024

TheSpyder commented May 3, 2024

TheSpyder left a comment

TheSpyder May 3, 2024

TheSpyder May 3, 2024

TheSpyder May 3, 2024

TheSpyder May 3, 2024

TheSpyder May 3, 2024

	body: Select Dataset content should be trimmed and filtered before parsing
	body: `block_formats` and `font_family_formats` configuration parsing did not trim spaces, resulting in untranslated strings. Patch contributed by andrewnicols

	return rawFormats.replace(/;$/, '').split(';');
	return rawFormats.split(';');

Trim titles in basic settings (#9598) #9599

Are you sure you want to change the base?

Trim titles in basic settings (#9598) #9599

Conversation

andrewnicols commented Apr 29, 2024 • edited

andrewnicols commented Apr 29, 2024

TheSpyder commented Apr 30, 2024

TheSpyder commented Apr 30, 2024

EkimChau commented Apr 30, 2024

andrewnicols left a comment

Choose a reason for hiding this comment

andrewnicols commented Apr 30, 2024

TheSpyder commented May 3, 2024

TheSpyder left a comment

Choose a reason for hiding this comment

TheSpyder May 3, 2024

Choose a reason for hiding this comment

TheSpyder May 3, 2024

Choose a reason for hiding this comment

TheSpyder May 3, 2024

Choose a reason for hiding this comment

TheSpyder May 3, 2024

Choose a reason for hiding this comment

TheSpyder May 3, 2024

Choose a reason for hiding this comment

andrewnicols commented Apr 29, 2024 •

edited