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

Removing toolbar plugin not possible (required by notification plugin) #491

Closed
rdeangelis83 opened this issue Jun 9, 2017 · 9 comments
Closed
Assignees
Labels
plugin:notification The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. type:bug A bug.
Milestone

Comments

@rdeangelis83
Copy link

rdeangelis83 commented Jun 9, 2017

Are you reporting a feature or a bug?

Bug

Provide detailed reproduction steps (if any)

  1. Create CKEditor config that removes the toolbar and the statusbar (elementspath)
    {removePlugins: 'elementspath, toolbar'}

We would like to provide our own toolbar ui.

Expected result

The toolbar will be removed from the CKEditor instance

Actual result

Error code: editor-plugin-required.
Object plugin: "toolbar" requiredBy: "notification"

The notification plugin has a dependency to the toolbar plugin. Removing also the notification plugin is not an option because a lot of other plugins have dependency to the notification plugin.

What is the actual result of the above steps?
CKEditor will be shown with his own toolbar.

Other details

  • Browser: Chrome
  • OS: Windows
  • CKEditor version: 4.7.0
  • Installed CKEditor plugins: standard-full
@mlewand
Copy link
Contributor

mlewand commented Jun 9, 2017

Hmm, that's actually a good point I also feel like notification shouldn't require toolbar button, however a quick look shows that it depends on it for positioning.

I'll leave it for confirmation, so that some1 will step in and do a bit deeper investigation.

@jonnott
Copy link

jonnott commented Jun 15, 2017

@wwalc @Reinmar @fredck ?

@jonnott
Copy link

jonnott commented Jun 28, 2017

+1

@jonnott
Copy link

jonnott commented Jun 30, 2017

Looking at http://docs.ckeditor.com/source/plugin39.html .. it's not even clear WHY the notification plugin has any programmatic need to require the toolbar plugin. Surely in its positioning calculations it could just detect whether a toolbar exists or not, and calculate accordingly. The supposed 'require'ment seems like nonsense..

@jonnott
Copy link

jonnott commented Jun 30, 2017

@mlewand Surely this is therefore quite easily resolvable?

@f1ames
Copy link
Contributor

f1ames commented Jul 3, 2017

From _layout function description:

Sets the position of the notification area based on the editor content, toolbar as well as viewport position and dimensions.

So it seems the only place where toolbar is used/accessed is the _layout function which uses it to calculate proper notification area position. It should be checked how absence of a toolbar affects those cases. Probably if there is no toolbar it should not be considered in a calculations as @jonnott pointed out.

@mlewand mlewand added this to the Backlog milestone Jul 3, 2017
@jonnott
Copy link

jonnott commented Jul 3, 2017

@f1ames I'm struggling to see where there is actually any use of toolbar code at all here.. maybe in function setBelowToolbar() { .. ), but that doesn't even seem to contain anything that's toolbar plugin-specific.

I'm at a loss to why this plugin even requires the toolbar plugin at all - can anyone point to the lines of code in question, or suggest what exactly would go wrong in this plugin's code if the toolbar plugin wasn't there??

@mlewand

@mlewand mlewand added status:confirmed An issue confirmed by the development team. type:bug A bug. plugin:notification The plugin which probably causes the issue. labels Jul 6, 2017
@msamsel msamsel self-assigned this Jul 6, 2017
@mlewand mlewand modified the milestones: 4.7.2, Backlog Aug 16, 2017
@jonnott
Copy link

jonnott commented Sep 26, 2017

So is this now fixed in the current release (4.7.2/3) ?

@f1ames
Copy link
Contributor

f1ames commented Sep 27, 2017

@jonnott, yes this particular issue had been fixed with release 4.7.2 (see Milestone), however to fully handle this issue, additional two fixes are required: #654, #678 which are targeted to 4.8.0 (you can see planned release date in the milestone details page).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin:notification The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. type:bug A bug.
Projects
None yet
Development

No branches or pull requests

5 participants