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

Make applyStyle and removeStyle hookable #3306

Closed
f1ames opened this issue Jul 31, 2019 · 4 comments
Closed

Make applyStyle and removeStyle hookable #3306

f1ames opened this issue Jul 31, 2019 · 4 comments
Assignees
Labels
changelog:api A changelog entry should be put in the API section of the changelog. status:confirmed An issue confirmed by the development team. type:feature A feature request.

Comments

@f1ames
Copy link
Contributor

f1ames commented Jul 31, 2019

Type of report

Feature request

Provide description of the new feature

Some of the features use commands (e.g. bold, italics, alignment, etc) which makes it easy to hook into with beforeCommandExec and afterCommandExec.

However, others use applyStyle and removeStyle (e.g. font, stylescombo, colorbutton, colordialog) which makes it impossible to hook into as there are no events or any other forms of intercepting the flow (apart from hacky methods proxying). This makes it quite difficult to integrate with other features.

So the idea is to make applyStyle and removeStyle methods hookable. We were thinking about refactoring using commands however it requires quite significant amount of work and may be problematic. So we decided to go with events which will be fired before/after styles applying/removing.

I'm just wondering if one event will be enough - something like beforeApplyStyle/removeApplyStyle (which will also allow canceling styles applying) or we could also add afterApplyStyle/afterApplyStyle? cc @jacekbogdanski

@f1ames f1ames added type:feature A feature request. status:confirmed An issue confirmed by the development team. changelog:api A changelog entry should be put in the API section of the changelog. core The issue is caused by the editor core code. labels Jul 31, 2019
@f1ames f1ames added this to the 4.13.0 milestone Jul 31, 2019
@jacekbogdanski
Copy link
Member

jacekbogdanski commented Jul 31, 2019

Fortunately, these methods don't return anything, so probably canceling them shouldn't cause side effects. Yeah, I think that additional events instead of dedicated methods will be much easier to implement (single place) with the same profit.

Just don't forget about passing style object into an event, otherwise, it will be useless.

after events looks redundant for me as if I'm correct styles method are called synchronously, so you can always attach a listener at the end of the event queue. In that case, maybe let's simplify naming into applyStyle/removeStyle event?

@jacekbogdanski jacekbogdanski self-assigned this Aug 1, 2019
@Comandeer
Copy link
Member

You can look into copyformatting plugin. We made there commands and events around applying styles. Maybe similar strategy can be used also for styles core?

@f1ames
Copy link
Contributor Author

f1ames commented Aug 30, 2019

After some testing (@jacekbogdanski 👍) we decided to go with commands (but in simplified version). Since few plugins overuse apply/removeStyle() methods (for example calling both methods on a single styles application) firing event will not be reliable.

So the idea is to provide commands which will be wrapping current code which applies styles for a given plugin (so it's kind of implementation/refactor per plugin basis). We do not want to try with any generic solution since it will require huge amount of work.

Each style should have own command, for example the font plugin which adds Font and FontSize combos, should have font and fontSize commands, which will be fired on style application (so probably somewhere there). If the same style is applied 2nd time, command should not be fired (since styles are not toggleable).

The command data object should contain applied style (if style is removed, it should be null or something similar).

Let's start with covering:

  1. font plugin
  2. colorbutton plugin

Depending on how it goes we may think of supporting format and stylescombo to have all "combo-like" plugins covered.

@msamsel msamsel self-assigned this Aug 30, 2019
@f1ames f1ames modified the milestones: 4.13.0, Next Sep 16, 2019
@f1ames f1ames modified the milestones: 4.13.1, 4.14.0 Sep 26, 2019
@f1ames f1ames added the target:major Any docs related issue that should be merged into a major branch. label Sep 26, 2019
@f1ames f1ames removed the target:major Any docs related issue that should be merged into a major branch. label Nov 12, 2019
@f1ames f1ames modified the milestones: 4.14.0, Iteration 2019-1 Dec 10, 2019
This was referenced Dec 11, 2019
@jacekbogdanski jacekbogdanski removed the core The issue is caused by the editor core code. label Dec 11, 2019
@jacekbogdanski
Copy link
Member

Extracted commands for colorbutton and font plugins separately, see #3727
#3728

Although, implementation details are still relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:api A changelog entry should be put in the API section of the changelog. status:confirmed An issue confirmed by the development team. type:feature A feature request.
Projects
None yet
Development

No branches or pull requests

4 participants