Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Trim titles in basic settings (#9598) #9599

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Trim titles in basic settings (#9598) #9599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/unreleased/tinymce-GH-9599-2024-04-30.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
project: tinymce
kind: Fixed
body: Select Dataset content should be trimmed and filtered before parsing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

time: 2024-04-30T11:26:37.663244+08:00
custom:
Issue: GH-9599
3 changes: 1 addition & 2 deletions modules/tinymce/src/core/test/ts/browser/FontSelectCustomTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { UiFinder } from '@ephox/agar';
import { describe, it } from '@ephox/bedrock-client';
import { Strings } from '@ephox/katamari';
import { SugarBody, TextContent } from '@ephox/sugar';
import { TinyHooks, TinySelections } from '@ephox/wrap-mcagar';
import { assert } from 'chai';
Expand All @@ -22,7 +21,7 @@ describe('browser.tinymce.core.FontSelectCustomTest', () => {

const assertSelectBoxDisplayValue = (title: string, expectedValue: string) => {
const selectBox = UiFinder.findIn(SugarBody.body(), `*[data-mce-name="${title}"]`).getOrDie();
const value = Strings.trim(TextContent.get(selectBox) ?? '');
const value = TextContent.get(selectBox) ?? '';
assert.equal(value, expectedValue, 'Should be the expected display value');
};

Expand Down
42 changes: 6 additions & 36 deletions modules/tinymce/src/themes/silver/main/ts/ui/core/complex/SelectDatasets.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,7 @@
import { Arr } from '@ephox/katamari';

import Editor from 'tinymce/core/api/Editor';

import { UiFactoryBackstageForStyleFormats } from '../../../backstage/StyleFormatsBackstage';

export interface BasicSelectItem {
readonly title: string;
readonly format: string;
readonly icon?: string;
}
import { BasicSelectItem, Delimiter, makeBasicSelectItems } from './utils/Select';

export interface BasicSelectDataset {
readonly type: 'basic';
Expand All @@ -21,47 +14,24 @@ export interface AdvancedSelectDataset extends UiFactoryBackstageForStyleFormats

export type SelectDataset = BasicSelectDataset | AdvancedSelectDataset;

const process = (rawFormats: string[]): BasicSelectItem[] => Arr.map(rawFormats, (item) => {
let title = item, format = item;
// Allow text=value block formats
const values = item.split('=');
if (values.length > 1) {
title = values[0];
format = values[1];
}

return { title, format };
});

const buildBasicStaticDataset = (data: Array<BasicSelectItem>): BasicSelectDataset => ({
type: 'basic',
data
});

export enum Delimiter {
SemiColon,
Space
}

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

const buildBasicSettingsDataset = (editor: Editor, settingName: 'block_formats' | 'font_family_formats' | 'font_size_formats', delimiter: Delimiter): BasicSelectDataset => {
// eslint-disable-next-line @tinymce/no-direct-editor-options
const rawFormats = editor.options.get(settingName);
const data = process(split(rawFormats, delimiter));
const data = makeBasicSelectItems(rawFormats, delimiter);
return {
type: 'basic',
data
data,
};
};

export {
buildBasicSettingsDataset,
buildBasicStaticDataset
buildBasicStaticDataset,
Delimiter,
BasicSelectItem
Comment on lines +35 to +36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};
43 changes: 43 additions & 0 deletions modules/tinymce/src/themes/silver/main/ts/ui/core/complex/utils/Select.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import { Arr } from '@ephox/katamari';

import { Strings} from '@ephox/katamari';

export interface BasicSelectItem {
readonly title: string;
readonly format: string;
readonly icon?: string;
}

export enum Delimiter {
SemiColon = ';',
Space = ' '
}

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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

if (values.length > 1) {
title = values[0];
format = values[1];
}

title = Strings.trim(title);
format = Strings.trim(format);

return { title, format };
});

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(';');

} else {
return rawFormats.split(' ');
}
};

const makeBasicSelectItems = (rawFormats: string, delimiter: Delimiter): BasicSelectItem[] => process(split(rawFormats, delimiter));

export {
makeBasicSelectItems
};
47 changes: 47 additions & 0 deletions modules/tinymce/src/themes/silver/test/ts/atomic/ui/core/complex/utils/SelectTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import { context, describe, it } from '@ephox/bedrock-client';
import { assert } from 'chai';

import { Delimiter, makeBasicSelectItems } from 'tinymce/themes/silver/ui/core/complex/utils/Select';

describe('atomic.tinymce.themes.silver.ui.core.complex.utils.Select', () => {
context('makeBasicSelectItems', () => {
it('Works with a semicolon delimiter', () => {
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assert.deepEqual(items, [
{ title: 'Paragraph', format: 'p' },
{ title: 'Heading 1', format: 'h1' },
{ title: 'Heading 2', format: 'h2' },
{ title: 'Heading 3', format: 'h3' },
{ title: 'Pre', format: 'pre' }
]);
});
it('Works with a space delimiter', () => {
const config = '8pt 10pt 12pt 14pt 18pt 24pt 36pt';
const items = makeBasicSelectItems(config, Delimiter.Space);
assert.isArray(items);
assert.lengthOf(items, 7);
assert.deepEqual(items, [
{ title: '8pt', format: '8pt' },
{ title: '10pt', format: '10pt' },
{ title: '12pt', format: '12pt' },
{ title: '14pt', format: '14pt' },
{ title: '18pt', format: '18pt' },
{ title: '24pt', format: '24pt' },
{ title: '36pt', format: '36pt' }
]);
});
it('Works with a mix of text/value items', () => {
const config = 'Paragraph = p ; pre';
const items = makeBasicSelectItems(config, Delimiter.SemiColon);
assert.isArray(items);
assert.lengthOf(items, 2);
assert.deepEqual(items, [
{ title: 'Paragraph', format: 'p' },
{ title: 'pre', format: 'pre' }
]);
});
});
});