Balloon Toolbar API is now available in plugin.init() #1402
Conversation
jacekbogdanski
changed the title
jacekbogdanski
force-pushed
the
t/1346
branch
2 times, most recently
from
bd22e0a to
da89f78
Compare
plugins/balloontoolbar/plugin.js
Outdated
| if ( !cssLoaded ) { | ||
| // Load fallback styles. | ||
| CKEDITOR.document.appendStyleSheet( this.path + 'skins/default.css' ); | ||
| CKEDITOR.document.appendStyleSheet( this.path + 'skins/' + CKEDITOR.skin.name + '/balloontoolbar.css' ); | ||
| cssLoaded = true; | ||
| } | ||
|
|
||
| // enable context api in plugin.init() |
There was a problem hiding this comment.
This is plugin.beforeInit not plugin.init. I suggest chanigng the comment to something like (also remember about referencing issues in the comment):
Initialize 'contextManager' so it is available in 'plugin.init' method for other plugins (#1346).
| @@ -0,0 +1,21 @@ | |||
| <textarea id="editor1" cols="10" rows="10"></textarea> | |||
There was a problem hiding this comment.
The convention is to just skip spaces or any - and _ in test names, so this file should be named contextapi.html (same for md file).
| CKEDITOR.replace( 'editor1', { | ||
| extraPlugins: 'testplugin' | ||
| } ); | ||
|
|
There was a problem hiding this comment.
Unnecessary space, I mean empty line 😄
| @@ -0,0 +1,11 @@ | |||
| @bender-ui: collapsed | |||
| @bender-tags: 4.8.0, feature, 1346, balloontoolbar | |||
There was a problem hiding this comment.
As 4.8.0 was already release, the target version of this fix is 4.8.1.
| @@ -0,0 +1,11 @@ | |||
| @bender-ui: collapsed | |||
| @bender-tags: 4.8.0, feature, 1346, balloontoolbar | |||
| @bender-ckeditor-plugins: wysiwygarea,toolbar,undo,basicstyles,notification,floatingspace,sourcearea,list,link | |||
There was a problem hiding this comment.
When creating tests, remember to include plugins which are only necessary for the tests to pass/fail. In this case, you could just skip all plugins as there is not user interaction with the editor.
However, it is better to make editor properly load (so it doesn't looks like test is broken) so you could use wysiwygarea,toolbar,basicstyles.
|
|
||
| Open console and check if `editor.balloonToolbars should be initialized` error has been thrown. | ||
|
|
||
| If not, test passed. |
There was a problem hiding this comment.
When creating tests description you may try to follow our common pattern for tests scenarios like:
| @@ -0,0 +1,26 @@ | |||
| /* bender-tags: balloontoolbar, 1346, context */ | |||
There was a problem hiding this comment.
I think context it to detailed, I would go with editor, balloontoolbar, 1346.
| ( function() { | ||
| 'use strict'; | ||
|
|
||
| bender.editor = {}; |
There was a problem hiding this comment.
bender.editor = {}; creates default (based on bender tags) CKEditor instance, before any tests are executed. As you are already creating an editor instance in test (bender.editorBot.create) you may simply remove this line.
| } ); | ||
|
|
||
| bender.editorBot.create( | ||
| { name: 'editor2', config: { extraPlugins: 'testplugin' } }, function noop( ) {} ); |
There was a problem hiding this comment.
This line should be indented (or you may simply put everything in the one line). Also I don't see any benefit in naming this noop function. You may simply use bender.editorBot.create( { ... }, function() {} );.
| @@ -0,0 +1,26 @@ | |||
| /* bender-tags: balloontoolbar, 1346, context */ | |||
There was a problem hiding this comment.
plugin.js is not very descriptive name for test suite, maybe something like contextinit.js (well, it will be nice if the naming will be consistent with manual test name, so you could go with contextapi)?
|
Could you also add suggested changelog entry? |
There was a problem hiding this comment.
Looks good. Just one more thing, when it comes to changelog entry it should be added directly to CHANGES.md (check out our dev-docs) not to PR description (I recall I told you it should be added to description, sorry for misguiadance 😅 ).
Please add changelog entry to CHANGES.md and we are ready to go.
|
Rebased on latest |
What is the purpose of this pull request?
Feature
Does your PR contain necessary tests?
All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.
This PR contains
What changes did you make?
Added balloon toolbar api context initialization to plugin.beforeInit().
Closes #1346
#1346: Fixed: Balloon Toolbar API should be exposed in plugin.init() of a required plugin.