Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

T/ckeditor5/1636: MultiCommand #179

Merged
merged 32 commits into from
Jun 28, 2019
Merged

T/ckeditor5/1636: MultiCommand #179

merged 32 commits into from
Jun 28, 2019

Conversation

jodator
Copy link
Contributor

@jodator jodator commented Jun 14, 2019

Suggested merge commit message (convention)

Feature: Introduce MultiCommand that allows to group many commands as one.


Additional information

This is part of IndentBlock PR.

The changes in core:

  1. Created Indent feature plugins:

    • Indent - typical glue plugin.
    • IndentEditing - introduces the "multi" 'indent' and 'outdent' commands.
    • IndentUI - introduces the 'indent' and 'outdent' toolbar buttons.

    We talked about defining Indent feature (buttons) with @Reinmar but I'm divided on where to put this feature. From one side this feature might not be needed by some - thus it might be better to put it in... 🥁 ...another repository as a separate ckeditor5-indent feature. But OTOH we will probably include this in most of the builds to use 'indent' and 'outdent' buttons.

  2. The MultiCommand that allows to register many commands under one alias.

    I've created the MultiCommand as a way to programmatically register other indentation commands under one command so the UI buttons would attach to one command only event thought there are voices against (by @pjasiun dunno @Reinmar voice on this).

    The command allows other features to register a child command (if it is defined in command collection). The MultiCommand changes its isEnabled state to true if any of registered commands is enabled. It also executes first enabled command (if any).

    I've created this command as we want to expose 'indent' and 'outdent' buttons that will enable to indent list and indent blocks. We already have 'indentList' (I'll skip the outdent as it is similar) command and a newly introduced 'indentBlock' command. We want to have one button for both (as in CKEditor 4). Now, I don't like the idea of overriding, ie indentBlock command by indentList command if the command is in list and not in a block. This isn't clean and might introduce some WTFs to others. Now the MultiCommand might be just InputCommand with the same interface as we have only one usage for such construct now and in forseable future. But I'm strongly against any ifs or other hacks in one commands that would tightly couple both features. (early stage POC with buttons)

    The MultiCommand allows to have any of indent feature to be present and does not require loading of other.

@coveralls
Copy link

coveralls commented Jun 14, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 0b23e78 on t/ckeditor5/1636 into 21b3fd3 on master.

@jodator jodator requested review from oleq and pjasiun June 14, 2019 11:51
@jodator
Copy link
Contributor Author

jodator commented Jun 14, 2019

I've invited you @pjasiun to review this alongisde @oleq just because the previous comments on this issue.

* @param {module:core/command~Command} command
*/
registerChildCommand( command ) {
this._childCommands.add( { command } );
Copy link
Member

Choose a reason for hiding this comment

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

Why do you wrap it with an object?

Copy link
Member

Choose a reason for hiding this comment

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

Because we use Collection? Then why using Collection?

Copy link
Contributor Author

@jodator jodator Jun 27, 2019

Choose a reason for hiding this comment

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

I think that I've started with collection to use some of its delegation powers but it turned out I use as an array. I'll change that.

*
* @param {module:core/command~Command} command
*/
registerChildCommand( command ) {
Copy link
Member

Choose a reason for hiding this comment

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

Just addChildCommand()?

Copy link
Member

Choose a reason for hiding this comment

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

Although, "register" reads quite well in the docs. So I don't have a strong opinion on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "register" implies something more than just adding it (like adding to a collection). So if it also reads well in the docs I'll leave it as is.

Copy link
Member

Choose a reason for hiding this comment

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

Good point. We actually start watching those commands.

src/multicommand.js Outdated Show resolved Hide resolved
src/multicommand.js Outdated Show resolved Hide resolved
src/multicommand.js Outdated Show resolved Hide resolved
src/multicommand.js Outdated Show resolved Hide resolved
src/indent/indent.js Outdated Show resolved Hide resolved
src/indent/indent.js Outdated Show resolved Hide resolved
src/indent/indent.js Outdated Show resolved Hide resolved
src/indent/indent.js Outdated Show resolved Hide resolved
src/indent/indent.js Outdated Show resolved Hide resolved
Reinmar and others added 14 commits June 26, 2019 15:02
Co-Authored-By: Piotrek Koszuliński <pkoszulinski@gmail.com>
Co-Authored-By: Piotrek Koszuliński <pkoszulinski@gmail.com>
Co-Authored-By: Piotrek Koszuliński <pkoszulinski@gmail.com>
Co-Authored-By: Piotrek Koszuliński <pkoszulinski@gmail.com>
Co-Authored-By: Piotrek Koszuliński <pkoszulinski@gmail.com>
Co-Authored-By: Piotrek Koszuliński <pkoszulinski@gmail.com>
Co-Authored-By: Piotrek Koszuliński <pkoszulinski@gmail.com>
Co-Authored-By: Piotrek Koszuliński <pkoszulinski@gmail.com>
Co-Authored-By: Piotrek Koszuliński <pkoszulinski@gmail.com>
@jodator jodator changed the title T/ckeditor5/1636: Indent buttons and commands. T/ckeditor5/1636: MultiCommand Jun 27, 2019
* @inheritDoc
*/
refresh() {
// Override base command refresh(): the command's state is changed when one of child commands changes states.
Copy link
Member

Choose a reason for hiding this comment

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

I became unsure about this. I'll report a followup, though.

@Reinmar Reinmar merged commit ebcbd01 into master Jun 28, 2019
@Reinmar Reinmar deleted the t/ckeditor5/1636 branch June 28, 2019 07:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants