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.

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

Implement block indentation feature #1636

Closed
Reinmar opened this issue Mar 19, 2019 · 11 comments
Closed

Implement block indentation feature #1636

Reinmar opened this issue Mar 19, 2019 · 11 comments
Assignees
Labels
type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Mar 19, 2019

Is this a bug report or feature request? (choose one)

🆕 Feature request

📃 Other details that might be useful

A popular feature of CKEditor 4 that we still don't have in CKEditor 5 is Indent Block.

This feature applies margin-left/right (depending on the content language) or a class (like indent-1/2/3) to the current block (e.g. a <p>).

It exposes indent/outdent buttons. It does not block the Tab key which people need to be able to leave the editor. However, it could use different keystrokes to allow indenting/outdenting the current block.

One more important aspect is that the indent/outdent buttons allow indenting/outdenting list items too. This is a bit tricky architecturally because the list feature is completely independent from this feature. This could be done by the list feature checking if there are indentBlock/outdentBlock commands and overriding their behaviour when the selection is in a list item.

Related features

There are two slightly similar feature requests, but which actually describe different features:

@Reinmar Reinmar added type:feature This issue reports a feature request (an idea for a new functionality or a missing option). status:confirmed labels Mar 19, 2019
@Reinmar Reinmar modified the milestones: iteration 23, next Mar 19, 2019
@jodator
Copy link
Contributor

jodator commented Jun 7, 2019

One more important aspect is that the indent/outdent buttons allow indenting/outdenting list items too. This is a bit tricky architecturally because the list feature is completely independent from this feature. This could be done by the list feature checking if there are indentBlock/outdentBlock commands and overriding their behaviour when the selection is in a list item.

I've been thinking about this part a bit and I've came to a conclusion that new API would be nice here but nothing more then building blocks ATM as I don't see scenarios for general API.

By building blocks I mean is to define a multi command that could aggregate many commands in order to expose single point for buttons.

The idea is to rename indentList and outdentList to indent and outdent (or add an alias for them).
Make them interact with indent command (the enabled state & execute).

The building blocks would be as follows:

const indentListCommand = new IndentListCommand();
editor.commands.add( 'indentList', indentListCommand );

const indentCommand = editor.commands.get( 'indent' );

indentCommand.registerChildCommand( indentListCommand );

Similarly the indentBlock command would be added to the 'indent' command.

The idea was that indent/outdent commands would be added by the core and either list or indent block feature could add their commands to that general one.

The MultiCommand would just glue them together so there would be only one button in the toolbar.

  • it will be enabled if any of commands is enabled
  • it will execute first command that is enabled
  • basically the two commands should not be enabled for the same selection

The small addition to this could be an API that would allow adding commands to some alias:

// list feature
editor.commands.add( 'indentList', new IndentListCommand() );
editor.commands.addMulti( 'indent', 'indentList' );

// block indent feature
editor.commands.add( 'indentBlock', new IndentBlockCommand() );
editor.commands.addMulti( 'indent', 'indentBlock' );

The creation of MultiCommand would be done by commands collection.


The other idea that came to my mind is aggregating commands in buttons itself but I don't have an idea for that API and from what I've seen that I features are defined as factories and you can have multiple instances of them in toolbar(s). So the multi command API looks like better suited for this case.


As @msamsel pointed out similar feature might be background color that will aggragate fontBackgroundColor and non-yet existing (if ever) tableCellBackgroundColor command.

// cc @pjasiun

@jodator
Copy link
Contributor

jodator commented Jun 7, 2019

The POC in action:

Peek 2019-06-07 11-09

@pjasiun
Copy link

pjasiun commented Jun 7, 2019

I am not sure that we need "building blocks", at the moment. List + indent block looks, for more like an exception than a common pattern. I do not think we should do it for background color/table cell background too, notice that for MS Word these are separate features:

background

This is why, I think, it should be a simple "if" in the block indent plugin which checks if the selection is in the list and if the list plugin as enabled, or the other way: list plugin can overwrite indent command execution, as @Reinmar suggested. Fortunately, the execute method of command is decodable.

I do not think we need any new mechanism here.

@jodator
Copy link
Contributor

jodator commented Jun 7, 2019

The table background was just an example so let's focus only on indent here.

This is why, I think, it should be a simple "if" in the block indent plugin which checks if the selection is in the list and if the list plugin as enabled, or the other way: list plugin can overwrite indent command execution, as @Reinmar suggested. Fortunately, the execute method of command is decodable.

We do need 3 plugins/features for it to do so as any of combination is valid IMO:

  • indent button with indent list & indent block
  • indent button with indent list only (no indent block plugin)
  • indent button with indent block only (no indent list - ie no lists at all)

So it would have to be

  • indent (probably in core or as a separate plugin for creating buttons and base commands)
  • indent list that overrides indent commands
  • indent block that overrides indent commands

I think that indent list and intend block doesn't have to know about each other why tie them?

So the simple if in any of list/block command is artificial and would require all plugins to be loaded in order to use one feature.

Also overriding other commands executions doesn't seems clean to me in this case. I can see that overriding default behavior of "enter" key in lists is valid but in here I don't think so.

@Reinmar
Copy link
Member Author

Reinmar commented Jun 26, 2019

I've been thinking about this too and I like the idea about MultiCommand. This is a nice implementation of the composite pattern and actually quite classic. So that looks fine.

My biggest problem was where to place the base indent stuff. I proposed ckeditor5-core because:

  • there's PendingActions already there,
  • in ckeditor5-ui we also include some base plugins.

But after talking with @oleq I think I'm changing my mind. All the plugins mentioned above are just utils and Indent will not be a util but an end-user feature (but without any default behaviour). So, I'd say that moving it to ckeditor5-indent despite the fact that it will be empty is the cleanest solution. And it's proven by CKEditor 4 already too.

@Reinmar
Copy link
Member Author

Reinmar commented Jun 26, 2019

So, I'd say that moving it to ckeditor5-indent despite the fact that it will be empty is the cleanest solution. And it's proven by CKEditor 4 already too.

In addition to the above – do we really need ckeditor5-indent and ckeditor5-block-indent? That'd be cleaner, but for convenience, we could merge both into ckeditor5-indent.

WDYT?

@oleq
Copy link
Member

oleq commented Jun 26, 2019

I'm for keeping

  • the generic code,
  • the block-indent,

in the same place (ckeditor5-indent). Sounds like a good place to aggregate indent features.

TBH, if possible, I'd go further and expose the minimal API in ckeditor5-list and move the indent list logic to ckeditor5-indent too. Not sure if that's possible, though.

@jodator
Copy link
Contributor

jodator commented Jun 26, 2019

So we could just rename the ckeditor5-block-indent repository to ckeditor5-indent and proceed with other changes?

@Reinmar
Copy link
Member Author

Reinmar commented Jun 26, 2019

So we could just rename the ckeditor5-block-indent repository to ckeditor5-indent and proceed with other changes?

If "other changes" mean moving there the code from ckeditor5-core, then yes :D

@Reinmar
Copy link
Member Author

Reinmar commented Jun 28, 2019

The first implementation landed in: http://github.com/ckeditor/ckeditor5-indent 🎉

@Reinmar Reinmar closed this as completed Jun 28, 2019
Reinmar added a commit to ckeditor/ckeditor5-theme-lark that referenced this issue Jul 1, 2019
Tests: Added indent and outdent icons to the iconset manual test (see ckeditor/ckeditor5#1636).
@mlewand
Copy link
Contributor

mlewand commented Jul 3, 2019

This feature was recently merged to the master branch 🎉

To enable it add the ckeditor5-indent plugin to your build.

Sample screencast:
Screencast showing increasing and decreasing indentation in paragraphs and lists using indent plugin toolbar buttons

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants