Handle alignment with classes #8996

maxbarnas · 2021-02-05T14:14:47Z

Suggested merge commit message (convention)

Feature (alignment): Add option to use classes instead of inline styles. Closes #8516.

Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

packages/ckeditor5-alignment/tests/alignmentediting.js

niegowski · 2021-02-16T13:20:04Z

packages/ckeditor5-alignment/docs/features/text-alignment.md

+			options: [ 'left', 'right' ],
+			classNames: [ 'my-align-left', 'my-align-right' ]


Maybe we could make this configuration as we do for image styles? https://ckeditor.com/docs/ckeditor5/latest/api/module_image_image-ImageConfig.html#member-styles

niegowski · 2021-02-16T13:33:37Z

packages/ckeditor5-alignment/src/alignmentediting.js

+		const view = {
+			key: 'style',
+			value: {
+				'text-align': option
+			}
+		};
+		const model = {
+			key: 'alignment',
+			value: option
+		};


I would inline those into the object below.

niegowski · 2021-02-16T13:38:25Z

packages/ckeditor5-alignment/src/alignmentediting.js

+	for ( const option of options ) {
+		definition.view[ option ] = {
+			key: 'class',
+			value: [ alignmentClassNames[ option ] ]


I'm not sure if this value should be an array?

Didn't we find it needed on the live review?

It was not needed after all. Thanks for pointing that out.

niegowski · 2021-02-16T13:47:42Z

packages/ckeditor5-alignment/tests/alignmentediting.js

+
+						setModelData( model, '<paragraph>[]x</paragraph>' );
+
+						expect( newEditor.getData() ).to.equal( '<p>x</p>' );


Maybe this line is not needed?

niegowski · 2021-02-19T16:52:10Z

packages/ckeditor5-alignment/src/alignmentediting.js

@@ -32,7 +32,8 @@ export default class AlignmentEditing extends Plugin {
 		super( editor );

 		editor.config.define( 'alignment', {
-			options: [ ...supportedOptions ]
+			options: [ ...defaultOptions ],
+			classNames: []


I guess this is not needed.

niegowski · 2021-02-19T17:18:20Z

packages/ckeditor5-alignment/src/alignmentediting.js

+		const enabledOptions = alignmentOptions.map( option => option.name ).filter( isSupported );
+		const classNameConfig = alignmentOptions.map( option => option.className );


I'm not sure if we should not stick to the list of objects and transform those to some helping maps.

niegowski · 2021-02-19T17:18:47Z

packages/ckeditor5-alignment/src/alignmentediting.js

+			const alignmentClassNames = classNameConfig.reduce( ( classNameMap, className, index ) => {
+				classNameMap[ enabledOptions[ index ] ] = className;
+
+				return classNameMap;
+			}, {} );


Maybe it would be more readable if we would use Object.fromEntries()

niegowski · 2021-02-19T17:23:38Z

packages/ckeditor5-alignment/src/utils.js

+	return Object.assign( {}, {
+		name: option
+	} );


Suggested change

return Object.assign( {}, {

name: option

} );

return {

name: option

};

niegowski · 2021-02-19T17:34:38Z

packages/ckeditor5-alignment/src/utils.js

+ * @returns {Array.<Object>} Normalized object holding the configuration.
+ */
+export function normalizeAlignmentOptions( configuredOptions = [] ) {
+	return configuredOptions.map( normalizeOption );


I would inline normalizeOption.

niegowski · 2021-02-19T17:34:59Z

packages/ckeditor5-alignment/src/utils.js

+	let optionObj;
+
+	if ( typeof option == 'string' ) {
+		optionObj = Object.assign( {}, { name: option } );


Suggested change

optionObj = Object.assign( {}, { name: option } );

optionObj = { name: option };

niegowski · 2021-02-19T17:39:20Z

packages/ckeditor5-alignment/src/utils.js

+	if ( typeof option == 'string' ) {
+		optionObj = Object.assign( {}, { name: option } );
+	} else {
+		optionObj = Object.assign( {}, defaultOptions[ index ], option );


Are you sure that those arrays are equal in length and have the same logic on the same indexes?

Also, I would suggest using:

optionObj = { ...someObject, ...someOverride, name: 'foo' }

niegowski · 2021-02-19T17:44:30Z

packages/ckeditor5-alignment/src/utils.js

+	const classNameAlreadyExists = optionObj.className && succeedingOptions.some(
+		// The `item.className` can be undefined. We shouldn't count it as a duplicate.
+		item => item.className &&
+				item.className == optionObj.className
+	);


Let's make it look cleaner.

Made it a notch more readable.

niegowski · 2021-02-19T17:46:29Z

packages/ckeditor5-alignment/src/utils.js

+	const nameAlreadyExists = succeedingOptions.some(
+		item => {
+			const optionName = item.name || item;
+
+			return optionName == optionObj.name;
+		} );


Suggested change

const nameAlreadyExists = succeedingOptions.some(

item => {

const optionName = item.name || item;

return optionName == optionObj.name;

} );

const nameAlreadyExists = succeedingOptions.some( item => {

const optionName = item.name || item;

return optionName == optionObj.name;

} );

Maybe we could do normalization from `"string" to { name: "string" }` in the first loop and then in the next loop normalize it?

maxbarnas · 2021-02-23T08:56:34Z

The PR is ready for another round of review.

Mgsy · 2021-02-23T10:26:28Z

Ready for testing, @FilipTokarski @LukaszGudel please take a look at it :)

packages/ckeditor5-alignment/src/alignment.js

niegowski · 2021-02-23T12:43:29Z

packages/ckeditor5-alignment/src/alignmentediting.js

+			const definition = buildClassDefinition( optionsToConvert );
+
+			editor.conversion.attributeToAttribute( definition );


This could be in one line.

niegowski · 2021-02-23T12:43:44Z

packages/ckeditor5-alignment/src/alignmentediting.js

+			const definition = buildDowncastInlineDefinition( optionNamesToConvert );

-		editor.conversion.attributeToAttribute( definition );
+			editor.conversion.for( 'downcast' ).attributeToAttribute( definition );


Same here - one line.

niegowski · 2021-02-23T12:50:53Z

packages/ckeditor5-alignment/src/alignmentediting.js

 // @private
-function _buildDefinition( options ) {
+function buildDowncastInlineDefinition( optionNames ) {


Maybe we could use options as in other definition builders (this way we will not need optionNamesToConvert)

niegowski · 2021-02-23T12:52:27Z

packages/ckeditor5-alignment/src/alignmentediting.js

+
+// Prepare upcast definitions for inline alignment styles.
+// @private
+function buildUpcastInlineDefinitions( optionNames ) {


I don't understand why this is needed now and wasn't needed previously.

Because downcasting to inline styles is now optional. That's why I had to divide upcast and downcast.

Ahh ok, and DowncastHelpers does not support that short notation for the definition

niegowski · 2021-02-23T12:57:27Z

packages/ckeditor5-alignment/src/utils.js

+ * @param {Array.<String|Object>} configuredOptions Alignment plugin configuration.
+ * @returns {Array.<Object>} Normalized object holding the configuration.
+ */
+export function normalizeAlignmentOptions( configuredOptions = [] ) {


Is this param optional?

Not anymore.

niegowski · 2021-02-23T13:00:11Z

packages/ckeditor5-alignment/src/utils.js

+			if ( typeof option == 'string' ) {
+				optionObj = { name: option };
+			} else {
+				optionObj = { ...optionNameToOptionMap[ option ], ...option };


This is confusing, what is it supposed to do?

Default value from optionNameToOptionMap[ option ] gets overwritten by option.

but option is not a string in this case so what it will take from the optionNameToOptionMap?

niegowski · 2021-02-23T13:07:10Z

packages/ckeditor5-alignment/src/utils.js

+			/**
+				 * The same `name` in one of the `alignment.options` was already declared.
+				 * Each `name` representing one alignment option can be set exactly once.
+				 *
+				 * @error alignment-config-name-already-defined
+				 * @param {Object} option First option that declares given `name`.
+				 * @param {Array.<String|Object>} allOptions Contents of `alignment.options`.
+				 */
+			throw new CKEditorError( 'alignment-config-name-already-defined', { option, allOptions } );


Please fix the indentation.

niegowski · 2021-02-23T13:07:17Z

packages/ckeditor5-alignment/src/utils.js

+			/**
+				 * The same `className` in one of the `alignment.options` was already declared.
+				 *
+				 * @error alignment-config-classname-already-defined
+				 * @param {Object} option First option that declares given `className`.
+				 * @param {Array.<String|Object>} allOptions Contents of `alignment.options`.
+				 */
+				throw new CKEditorError( 'alignment-config-classname-already-defined', { option, allOptions } );


Please fix the indentation.

niegowski · 2021-02-23T13:07:57Z

packages/ckeditor5-alignment/tests/alignmentediting.js

@@ -12,19 +12,19 @@ import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtest
 import { getData as getModelData, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';

 import AlignmentCommand from '../src/alignmentcommand';
+import { CKEditorError } from '../../../src/utils';


Seems to be an invalid import path.

niegowski · 2021-02-23T14:25:11Z

packages/ckeditor5-alignment/src/alignmentediting.js

@@ -63,17 +60,13 @@ export default class AlignmentEditing extends Plugin {

 		if ( shouldUseClasses ) {
 			// Downcast to only to classes.


This comment is not reflecting what the code does - it's converting both ways, not only downcasting.

niegowski · 2021-02-23T14:27:07Z

packages/ckeditor5-alignment/src/alignmentediting.js

@@ -86,11 +79,12 @@ export default class AlignmentEditing extends Plugin {

 // Prepare downcast conversion definition for inline alignment styling.
 // @private
-function buildDowncastInlineDefinition( optionNames ) {
+function buildDowncastInlineDefinition( options ) {
+	const optionNames = options.map( option => option.name );


Maybe we could inline it?

There are two places in this function using it. Does it make sense to inline it and repeat the same operation twice?

The first one uses a list of names, and the second one is iterating itself

niegowski · 2021-02-23T14:27:47Z

packages/ckeditor5-alignment/src/alignmentediting.js

@@ -109,7 +103,8 @@ function buildDowncastInlineDefinition( optionNames ) {

 // Prepare upcast definitions for inline alignment styles.
 // @private
-function buildUpcastInlineDefinitions( optionNames ) {
+function buildUpcastInlineDefinitions( options ) {
+	const optionNames = options.map( option => option.name );


This is not needed, you could access the option name in the loop below.

LukaszGudel · 2021-02-23T14:40:11Z

While testing this feature I've added this alignment config:

		alignment: {
			options: [
				'left',
				'right',
				{ name: 'center', className: 'AlignCenter' },
			]
		},

I know that it is incorrect to mix inlines and classes. Editor is opening with the content but it seems like there are some side effects. Every block element has class right and every table/widget has inline style float: right. If I change to RTL then every block element has class 'left' and tables are floating to the left. I've tested this on all-features manual test.

IMO even warning in console about mixed config should help any developer who is using these config options incorrectly.

niegowski · 2021-02-23T14:38:08Z

packages/ckeditor5-alignment/tests/alignmentediting.js

-		beforeEach( () => {
-			return VirtualTestEditor
+		beforeEach( async () => {
+			const newEditor = await VirtualTestEditor


newEditor const is not needed, you could assign it directly to editor.

niegowski · 2021-02-23T14:47:10Z

packages/ckeditor5-alignment/tests/alignmentediting.js

@@ -175,6 +225,34 @@ describe( 'AlignmentEditing', () => {

 			expect( editor.getData() ).to.equal( '<p style="text-align:center;">x</p>' );
 		} );
+
+		it( 'uses class when classNames is set', async () => {


It would be good to have a data upcast also covered.

niegowski · 2021-02-23T14:49:07Z

packages/ckeditor5-alignment/tests/utils.js

@@ -3,7 +3,8 @@
 * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
 */

-import { isDefault, isSupported, supportedOptions } from '../src/utils';
+import { CKEditorError } from '../../../src/utils';


This import seems to be invalid.

niegowski · 2021-02-23T14:50:45Z

packages/ckeditor5-alignment/tests/alignmentediting.js

+				} );
+			} );
+
+			describe( 'className property', () => {


Maybe those tests should be in the other test file where we test normalization?

LukaszGudel · 2021-02-23T15:24:38Z

Use config to use classes instead of inline styles.
Set data by calling:

editor.setData('<p style="text-align:center">Hello world</p><table><tr><td style="text-align:center"><p>Hello</p></td></tr></table>')

Result:

<p class="AlignCenter">Hello world</p>
<figure class="table"><table><tbody><tr><td style="text-align:center;">Hello</td></tr></tbody></table></figure>

Paragraph's style will convert to class but <td> style will not change.

LukaszGudel · 2021-02-23T15:34:11Z

CKEditor4 can output this html:

<table border="1" cellpadding="1" cellspacing="1" style="width:500px">
	<tbody>
		<tr>
			<td>Hello</td>
			<td class="AlignRight">World</td>
		</tr>
		<tr>
			<td class="AlignCenter">Foo</td>
			<td class="AlignJustify">Bar</td>
		</tr>
	</tbody>
</table>

but it looks like <td> classes are ignored in CKEditor5.

maxbarnas · 2021-02-23T16:25:30Z

editor.setData('<p style="text-align:center">Hello world</p><table><tr><td style="text-align:center"><p>Hello</p></td></tr></table>')

This is interesting. If you do the same with the inline styles (so no alignment.options), you'll get the same thing. Seems like our implementation (and I mean even pre-className version) does not accept alignment directly on <td>.

If you manually align the contents of the table cell, the cell will get a paragraph, and that paragraph will be styled. And this kind of contents will be correctly handled by setData() and getData().

niegowski · 2021-02-23T16:26:35Z

This is interesting. If you do the same with the inline styles (so no alignment.options), you'll get the same thing. Seems like our implementation (and I mean even pre-className version) does not accept alignment directly on <td>.

If you manually align the contents of the table cell, the cell will get a paragraph, and that paragraph will be styled. And this kind of contents will be correctly handled by setData() and getData().

Alignment in table cells is handled by the table cell styles feature.

niegowski · 2021-02-23T17:19:55Z

packages/ckeditor5-alignment/docs/features/text-alignment.md

+By default alignment is set inline using `text-align` CSS property. If you wish the feature to output more semantic content that uses classes instead of inline styles, you can specify class names by using the  `className` property in `config.alignment.options` and style them by using a stylesheet.
+
+<info-box>
+	Once you decide to use classes for the alignment, you must define `className` for **all** alignment entries in {@link module:alignment/alignment~AlignmentConfig#options `config.alignment.options`}.


There is no validation for this.

LukaszGudel · 2021-02-24T09:02:02Z

Overall, this feature seems to be fine 👍 I did not find weird behaviors, other than cases with alignments.

FilipTokarski · 2021-02-24T09:06:02Z

Same here, seems to be fine 👍

maxbarnas commented Feb 11, 2021

View reviewed changes

packages/ckeditor5-alignment/tests/alignmentediting.js Outdated Show resolved Hide resolved

maxbarnas marked this pull request as ready for review February 12, 2021 08:48

maxbarnas force-pushed the i/8516 branch from 1487bed to 329afcd Compare February 16, 2021 08:47

niegowski reviewed Feb 16, 2021

View reviewed changes

maxbarnas added 9 commits February 17, 2021 11:18

Typo.

f97ba0b

Handle alignment with classes.

159b3cf

Updated tests.

0f93c31

Update tests, update exception params.

d9ba476

Docs.

02c82ad

Tests.

a018c19

Remove unused code.

5a52dde

Docs cleanup.

950bd75

Fix wording.

226b937

maxbarnas force-pushed the i/8516 branch from f7b7d07 to 226b937 Compare February 17, 2021 10:18

maxbarnas added 6 commits February 17, 2021 22:46

Normalize config. Clean up async tests.

08a4365

Review fixes.

1ff1d30

Update failing tests.

38e0096

Add tests for normalizeAlignmentOptions.

77d95ae

Update docs and tests.

8fe8dad

Update API docs.

5685f43

niegowski reviewed Feb 19, 2021

View reviewed changes

maxbarnas added 2 commits February 21, 2021 22:01

Review fixes.

442cca8

Make the duplicate className detection more readable.

30a3c67

maxbarnas force-pushed the i/8516 branch from f83e9f3 to 30a3c67 Compare February 21, 2021 21:18

maxbarnas added 4 commits February 21, 2021 23:17

Remove redundant array.

f5ccfa8

Clean up tests.

dce3a13

Cleanup of defaults.

a1677cc

Update docs.

e16be30

maxbarnas requested a review from niegowski February 22, 2021 11:28

Missing comma.

036cf4b

niegowski requested changes Feb 23, 2021

View reviewed changes

Review fixes.

864f393

niegowski reviewed Feb 23, 2021

View reviewed changes

niegowski requested changes Feb 23, 2021

View reviewed changes

Review fixes part 2.

045e5f7

niegowski reviewed Feb 23, 2021

View reviewed changes

Throw exception when className is not provided for all the options.

cbcaa1e

maxbarnas force-pushed the i/8516 branch from 0c022f4 to cbcaa1e Compare February 24, 2021 07:58

maxbarnas requested a review from niegowski February 24, 2021 07:59

Code refactoring.

f9ee5f4

niegowski merged commit 638543b into master Feb 26, 2021

niegowski deleted the i/8516 branch February 26, 2021 13:43

		options: [ 'left', 'right' ],
		classNames: [ 'my-align-left', 'my-align-right' ]


		setModelData( model, '<paragraph>[]x</paragraph>' );

		expect( newEditor.getData() ).to.equal( '<p>x</p>' );

		const enabledOptions = alignmentOptions.map( option => option.name ).filter( isSupported );
		const classNameConfig = alignmentOptions.map( option => option.className );

	optionObj = Object.assign( {}, { name: option } );
	optionObj = { name: option };

		const definition = buildClassDefinition( optionsToConvert );

		editor.conversion.attributeToAttribute( definition );

		@@ -63,17 +60,13 @@ export default class AlignmentEditing extends Plugin {

		if ( shouldUseClasses ) {
		// Downcast to only to classes.

Handle alignment with classes #8996

Handle alignment with classes #8996

Conversation

maxbarnas commented Feb 5, 2021 • edited Loading

Suggested merge commit message (convention)

Additional information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxbarnas commented Feb 23, 2021

Mgsy commented Feb 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaszGudel commented Feb 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaszGudel commented Feb 23, 2021

Result:

LukaszGudel commented Feb 23, 2021 • edited Loading

maxbarnas commented Feb 23, 2021 • edited Loading

niegowski commented Feb 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaszGudel commented Feb 24, 2021

FilipTokarski commented Feb 24, 2021 • edited Loading

maxbarnas commented Feb 5, 2021 •

edited

Loading

LukaszGudel commented Feb 23, 2021 •

edited

Loading

maxbarnas commented Feb 23, 2021 •

edited

Loading

FilipTokarski commented Feb 24, 2021 •

edited

Loading