T/53a: Split "heading" command into independent commands. #58

oleq · 2017-03-07T15:19:13Z

Suggested merge commit message (convention)

Feature: Split "heading" command into independent commands. Closes ckeditor/ckeditor5#2435. Closes ckeditor/ckeditor5#2442. Closes ckeditor/ckeditor5#2437.

BREAKING CHANGE: The "heading" command is no longer available. Replaced by "heading1", "heading2", "heading3" and "paragraph".
BREAKING CHANGE: Heading plugin requires Paragraph to work properly (ParagraphCommand registered as "paragraph" in editor.commands).
BREAKING CHANGE: config.heading.options format has changed. The valid HeadingOption syntax is now { modelElement: 'heading1', viewElement: 'h1', title: 'Heading 1' }.

Additional information

Requires ckeditor/ckeditor5-paragraph#15.

oleq · 2017-03-08T14:20:36Z

self.R-. There's no need for HeadingEngine exposing #commands collection. It turned out it's pretty useless when dealing with https://github.com/ckeditor/ckeditor5-heading/issues/38.

Reinmar · 2017-03-08T15:35:36Z

src/heading.js

+
+		for ( let option of options ) {
+			// Add the option to the collection.
+			dropdownItems.add( new Model( {


Does it have to be a model instance? Can't this be a simple object?

Binding requires observable. We could pass an object here and convert it into observable in createlistdropdown. But this is beyond the scope of this issue as it didn't change this part.

OTOH I'm not sure whether would make more sense. There could be cases when a feature using the dropdown might want to play with models in #items immediately, i.e. bind its attributes to something else.

If we enforce that #items is Collection.<Object>, developers would need to pick those models from the dropdown after createListDropdown was called which is not very convenient.

Besides, what would ListView look like then if #items would be Collection.<Object>? Where to store the "intermediate models" created in createListDropdown()? Because there must be models for Model->ListItemView bindings.

Reinmar · 2017-03-08T15:39:46Z

src/heading.js

+			// Add the option to the collection.
+			dropdownItems.add( new Model( {
+				modelElement: option.modelElement,
+				label: option.title


This label = title doesn't look good.

I think that the name of the option property should be as generic as possible because we'll have the same problem all around our code. I wonder what's better – label, title or perhaps name. We have the same problem in ImageStyles and it would be good to find a naming convention fitting all such scenarios.

It may not look good but OTOH it makes sense. "Label" is a label in DOM, in the UI component. "Title" is any kind of title, it could be a label, but also aria attribute or plain text. This is where two of them meet.

OK, so if you find "title" more generic, then let's use it. Then, we're also fine in the ImageStyles which uses the same property name.

Reinmar · 2017-03-08T15:43:35Z

src/heading.js


 		// Register UI component.
 		editor.ui.componentFactory.add( 'headings', ( locale ) => {
 			const dropdown = createListDropdown( dropdownModel, locale );

 			// Execute command when an item from the dropdown is selected.
-			this.listenTo( dropdown, 'execute', ( { source: { id } } ) => {
-				editor.execute( 'heading', { id } );
+			this.listenTo( dropdown, 'execute', ( { source: { modelElement } } ) => {


The param of the callback is impossible for me to parse. Please use a more human-readable solution.

Learn destructuring then ;-)

I know destructuring and it doesn't help. Which means that the code is wrong.

Reinmar · 2017-03-08T15:44:17Z

src/heading.js

-			this.listenTo( dropdown, 'execute', ( { source: { id } } ) => {
-				editor.execute( 'heading', { id } );
+			this.listenTo( dropdown, 'execute', ( { source: { modelElement } } ) => {
+				editor.execute( modelElement );


If you execute this, then it's a commandName not modelElement.

Reinmar · 2017-03-08T15:46:52Z

src/heading.js

+		return editor.config.get( 'heading.options' ).map( option => {
+			let title;
+
+			if ( option.modelElement == 'paragraph' ) {


Why this condition? Can't it go through the usual:

title = localizedTitles[ option.title ];

The option with modelElement == 'paragraph' has no title property.

I meant – why it doesn't have that property? It doesn't make sense that it has viewElement. It doesn't also make sense that it has modelElement – it could be replaced with defaultBlock: true or something (but we can stick to modelElement – I'm ok with this). But it certainly can have title.

Actually, without it it's not localizable AFAICS. I mean – it can be localized through translations, but you can't use custom title.

Reinmar · 2017-03-08T16:13:04Z

src/headingcommand.js

 					}
 				}
 				// When applying new option.
 				else {
-					batch.rename( element, id );
+					batch.rename( element, this.modelElement );


Like in the paragraph command – you can skip renaming elements which are already named correctly.

Reinmar · 2017-03-08T16:13:44Z

src/headingcommand.js

 				}
 			}

 			// If range's selection start/end is placed directly in renamed block - we need to restore it's position
 			// after renaming, because renaming puts new element there.
-			doc.selection.setRanges( ranges, isSelectionBackward );
+			selection.setRanges( ranges, isSelectionBackward );


If everything else works correctly, this should be unnecessary. This command does not change the selection.

PS. This might've been necessary in the past when renaming wasn't an operation because then the live selection transformations might not have been able to keep the selection position.

Reinmar · 2017-03-08T16:18:07Z

src/headingengine.js

+					.toElement( option.modelElement );
+
+				// Register the heading command for this option.
+				command = new HeadingCommand( editor, option );


This variable is unnecessary.

HeadingCommand. Re-enabled command toggle. Renames in config.heading.options.

…ement. Refactoring in Heading after removal of ParagraphCommand#title.

…alization.

…r paragraph option. Enhanced tests.

oleq added status:review- and removed status:review- labels Mar 8, 2017

Reinmar reviewed Mar 8, 2017

View reviewed changes

oleq mentioned this pull request Mar 9, 2017

T/38: Enabled items styling in the headings toolbar dropdown #59

Merged

Reinmar requested a review from szymonkups March 9, 2017 13:01

oleq added 6 commits March 10, 2017 15:54

Split the HeadingCommand.

e76a030

Integration with ParagraphCommand. Use getSelectedBlocks() in

811e3d7

HeadingCommand. Re-enabled command toggle. Renames in config.heading.options.

Removed obsolote code from HeadingCommand tests. Docs refactoring.

611dcf3

HeadingCommand passes selection to the ParagraphCommand instead of el…

cb71790

…ement. Refactoring in Heading after removal of ParagraphCommand#title.

Removed HeadingEngine#commands. Simplified config.heading.options loc…

7b2c1c8

…alization.

Code refac. Deobfuscated some pieces of code. Allowed custom title fo…

65ac56c

…r paragraph option. Enhanced tests.

oleq force-pushed the t/53a branch from 4184ed5 to 65ac56c Compare March 10, 2017 14:55

szymonkups merged commit 7a8f6f0 into master Mar 13, 2017

szymonkups deleted the t/53a branch March 13, 2017 10:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T/53a: Split "heading" command into independent commands. #58

T/53a: Split "heading" command into independent commands. #58

oleq commented Mar 7, 2017 •

edited by pomek

Loading

oleq commented Mar 8, 2017 •

edited

Loading

Reinmar Mar 8, 2017

oleq Mar 8, 2017

oleq Mar 9, 2017 •

edited

Loading

Reinmar Mar 8, 2017

oleq Mar 8, 2017

Reinmar Mar 8, 2017

Reinmar Mar 8, 2017 •

edited

Loading

oleq Mar 9, 2017

Reinmar Mar 9, 2017

Reinmar Mar 8, 2017

Reinmar Mar 8, 2017

oleq Mar 8, 2017

Reinmar Mar 8, 2017

Reinmar Mar 8, 2017 •

edited

Loading

Reinmar Mar 8, 2017

Reinmar Mar 8, 2017

Reinmar Mar 8, 2017

Reinmar Mar 8, 2017

T/53a: Split "heading" command into independent commands. #58

T/53a: Split "heading" command into independent commands. #58

Conversation

oleq commented Mar 7, 2017 • edited by pomek Loading

Suggested merge commit message (convention)

Additional information

oleq commented Mar 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleq Mar 9, 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

Reinmar Mar 8, 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

Reinmar Mar 8, 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

oleq commented Mar 7, 2017 •

edited by pomek

Loading

oleq commented Mar 8, 2017 •

edited

Loading

oleq Mar 9, 2017 •

edited

Loading

Reinmar Mar 8, 2017 •

edited

Loading

Reinmar Mar 8, 2017 •

edited

Loading