t/33: Enabled configuration and localisation of headings #57

oleq · 2017-02-17T11:04:47Z

Suggested merge commit message (convention)

Feature: Enabled configuration and localization of headings. Closes ckeditor/ckeditor5#2418.

NOTE: Added config.heading.formats and config.heading.defaultFormatId config options.

oleq · 2017-02-17T11:08:11Z

tests/heading.js

+			beforeEach( () => {
+				const editorElement = document.createElement( 'div' );
+
+				const spy = testUtils.sinon.stub( Locale.prototype, '_t' ).returns( 'foo' );


Since items are localized in the heading plugin before stubbed editor.t would work, this is the only way I see to test it. TBH, I don't like it and I wish we had some easier way to assert localization because it will repeat in many places.

…`t( String )` definitions.

…tial config.heading.formats value.

Reinmar · 2017-02-19T11:43:38Z

src/heading.js

 			withText: true,
 			items: collection
 		} );

 		// Bind dropdown model to command.
 		dropdownModel.bind( 'isEnabled' ).to( command, 'isEnabled' );
-		dropdownModel.bind( 'label' ).to( command, 'value', format => format.label );
+		dropdownModel.bind( 'label' ).to( command, 'value', ( { label } ) => label );


The previous version was much easier to read.

It's a matter of preference, I suppose. It's easier for me to read the second one.

As with every preference, I can't say that your's weird, but... it is :D.

My way of thinking is – format => format.label tells you that a format is turned into just its label. With the deconstruction pattern there you lose the context (the format). It's burried deep in the binding that you did, so the callback may be far from intiuitive at first.

Reinmar · 2017-02-19T11:44:37Z

src/heading.js

@@ -40,23 +40,22 @@ export default class Heading extends Plugin {
 		const collection = new Collection();

 		// Add formats to collection.
-		for ( let format of formats ) {
+		for ( let { id, label } of formats ) {


If you really need to change this code (in this PR), then be comprehensive – this can be a const.

Reinmar · 2017-02-19T11:48:05Z

src/headingengine.js

+	 * @protected
+	 * @type {Array.<module:heading/headingcommand~HeadingFormat>}
+	 */
+	get _formats() {


https://github.com/ckeditor/ckeditor5-design/wiki/Code-Style#order-within-class-definition – getters and setters go before methods

OTOH, this more deserves to be a method – _getLocalizedFormats(). Plus, even the API doc is misleading now because it doesn't explain why this getter exists in the first place.

Reinmar · 2017-02-19T11:50:31Z

src/headingengine.js

+		editor.config.define( 'heading', {
+			formats: [
+				{ id: 'paragraph', element: 'p', label: labels.Paragraph },
+				{ id: 'heading1', element: 'h2', label: labels[ 'Heading 1' ] },


If the _formats getter localizes these labels, why are you using localized versions here too?

Note that two things must be localized:

Default config, which is editor.config.define( 'heading', { formats: [ ... ] } ).

User config, which is editor.config.get( 'heading.formats' ).

Reinmar · 2017-02-19T11:54:46Z

src/headingengine.js

+		 * @protected
+		 * @member {Object} #_localizedFormatLabels
+		 */
+		const labels = this._localizedFormatLabels = {


Must this be a property? It'd be a good private variable of this module.

Rationale – don't bloat the class.

Because of the above #57 (comment). Both default config and user config must be localized. The first is localized in constructor(), the second in init(). I cannot share these labels in any other way because they require editor.t() to be created.

User config can be localised together with the default config when they are processed by the _getLocalizedOptions() method (currently _options property, but I commented that it shouldn't be a property). The default config doesn't have to be localized beforehand.

You're right about the other thing, though. The localized labels cannot be defined in the module scope, because they require the t() function. I missed that.

Btw, this property also shouldn't use the word "format".

I'm thinking about the localization of user config and I'm afraid about one thing. Perhaps it's an edge case, but still...

Let's say you want editor in language X. So the _localizedOptionLabels contains e.g. map like:

Paragraph: 'X Paragraph', 'Heading 1': 'X Heading 1', ...

Now, someone also specifies config.heading.options, but want to use English strings there because for a weird case it make sense to keep them in English. But this turns to be impossible because they are automatically localized using _localizedOptionLabels mapping.

I hope that this case is really that edgy as it sounds now and can be ignored.

PS. This applies to the current implementation and the one proposed by me above.

If they want to use English strings so badly, they can write them with a ZWSP at the end, so _localizedOptionLabels does not translate them to current editor language and just passes them thru =)

It's an edge case, really. When someone translates the entire app but some labels.

I know :D I thought about the same "solution".

So I guess it's all fine. But this can still be improved in the code:

User config can be localised together with the default config when they are processed by the _getLocalizedOptions() method (currently _options property, but I commented that it shouldn't be a property). The default config doesn't have to be localized beforehand.

Reinmar · 2017-02-19T12:00:37Z

src/headingengine.js


 		if ( enterCommand ) {
 			this.listenTo( enterCommand, 'afterExecute', ( evt, data ) => {
 				const positionParent = editor.document.selection.getFirstPosition().parent;
 				const batch = data.batch;
-				const isHeading = formats.some( ( format ) => format.id == positionParent.name );
+				const isHeading = formats.some( ( { id } ) => id == positionParent.name );


Please revert this. The previous version was more readable.

Reinmar · 2017-02-19T13:02:47Z

Please consider https://github.com/ckeditor/ckeditor5-heading/issues/52. You added heading.formats while we'd like to get rid of that word in this context (it's a leftover from the time this was a formats feature).

What I don't know is how could the option be named ;> heading.options?

Reinmar · 2017-02-19T13:04:43Z

Last but not least – do we need to explicitly define the paragraph option? I wanted to ask the question because you may have a more concrete opinion about that, but personally I think that removing it from the config may only make things more complicated.

Reinmar · 2017-02-19T13:22:07Z

OK, that wasn't all yet :D.

config.heading.defaultFormatId config options.

We don't need this option yet. And I don't think that anyone will ever need it. Even if your editor implements some oddest editing use case, the <paragraph> element should still be the default editing block.

fredck · 2017-02-20T10:47:45Z

What I don't know is how could the option be named ;> heading.options?

+1 Wanted to say exactly the same :)

Reinmar · 2017-02-28T21:54:42Z

src/headingengine.js

+				{ id: 'heading2', element: 'h3', label: labels[ 'Heading 2' ] },
+				{ id: 'heading3', element: 'h4', label: labels[ 'Heading 3' ] }
+			],
+			defaultOptionId: 'paragraph'


Still to be removed.

Reinmar · 2017-02-28T21:54:58Z

src/headingengine.js

+	 * @protected
+	 * @type {Array.<module:heading/headingcommand~HeadingOption>}
+	 */
+	get _options() {


Still to not be a getter.

Take it easy ;-) It's a step–by–step refac.

Reinmar · 2017-03-01T09:53:49Z

src/headingengine.js

+		 * @readonly
+		 * @member {Array.<module:heading/headingcommand~HeadingOption>} #_cachedLocalizedOptions
+		 */
+		return ( this._cachedLocalizedOptions = editor.config.get( 'heading.options' )


Srsly, a multiline assignment combined with a return statement? Please, de-obfuscate this :P

Reinmar · 2017-03-01T17:00:25Z

I changed the message to:

Feature: Enabled configuration and localization of available headings. See config.heading.options. Closes #33.

BREAKING CHANGES: The heading command now accepts id option, not formatId.

oleq added 2 commits February 17, 2017 10:18

Introduced editor.config#formats and editor.config#defaultFormatId.

fc02f42

Enabled localization of heading button and item labels.

13381cf

oleq commented Feb 17, 2017

View reviewed changes

oleq added 3 commits February 17, 2017 13:42

Fixed: Heading labels not properly localized due to lack of explicit …

8874212

…`t( String )` definitions.

Tests: Refactoring. Additional tests to check the localization of ini…

905ef0e

…tial config.heading.formats value.

Code refactoring.

866b6ee

Reinmar reviewed Feb 19, 2017

View reviewed changes

Reinmar added the status:review- label Feb 19, 2017

oleq added 2 commits February 28, 2017 16:38

Code refactoring.

28b9621

Renamed 'format'->'option' in the entire ckeditor5-heading package.

0458aa2

Reinmar reviewed Feb 28, 2017

View reviewed changes

oleq added 3 commits March 1, 2017 10:10

Changed _options getter to _getLocalizedOptions() in HeadingEngine.

e477303

Code refactoring in HeadingCommand: 'format'->'option'.

7158d5b

Removed config.heading.defaultOptionId.

5602660

oleq removed the status:review- label Mar 1, 2017

oleq mentioned this pull request Mar 1, 2017

Updated Autoformat plugin after changes in Heading plugin API. ckeditor/ckeditor5-autoformat#21

Merged

Reinmar reviewed Mar 1, 2017

View reviewed changes

Code refactoring in HeadingEngine Plugin.

f8a04a3

Reinmar merged commit de07a0c into master Mar 1, 2017

Reinmar deleted the t/33 branch March 1, 2017 17:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

t/33: Enabled configuration and localisation of headings #57

t/33: Enabled configuration and localisation of headings #57

oleq commented Feb 17, 2017 •

edited by pomek

Loading

oleq Feb 17, 2017

Reinmar Feb 19, 2017

oleq Feb 28, 2017

Reinmar Mar 1, 2017

Reinmar Feb 19, 2017 •

edited

Loading

Reinmar Feb 19, 2017

Reinmar Feb 19, 2017

Reinmar Feb 19, 2017

oleq Feb 28, 2017

Reinmar Feb 19, 2017

Reinmar Feb 19, 2017

oleq Feb 28, 2017

Reinmar Feb 28, 2017

Reinmar Feb 28, 2017

Reinmar Feb 28, 2017

oleq Mar 1, 2017

oleq Mar 1, 2017

Reinmar Mar 1, 2017

Reinmar Feb 19, 2017

Reinmar commented Feb 19, 2017

Reinmar commented Feb 19, 2017

Reinmar commented Feb 19, 2017

fredck commented Feb 20, 2017

Reinmar Feb 28, 2017

Reinmar Feb 28, 2017

oleq Mar 1, 2017

Reinmar Mar 1, 2017

Reinmar commented Mar 1, 2017

t/33: Enabled configuration and localisation of headings #57

t/33: Enabled configuration and localisation of headings #57

Conversation

oleq commented Feb 17, 2017 • edited by pomek Loading

Suggested merge commit message (convention)

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

Reinmar Feb 19, 2017 • edited Loading

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

Reinmar commented Feb 19, 2017

Reinmar commented Feb 19, 2017

Reinmar commented Feb 19, 2017

fredck commented Feb 20, 2017

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

Reinmar commented Mar 1, 2017

oleq commented Feb 17, 2017 •

edited by pomek

Loading

Reinmar Feb 19, 2017 •

edited

Loading