Added possibility to use custom icons in buttons #1531
Conversation
tests/plugins/button/buttonicon.js
Outdated
| assertIcon( | ||
| { extraPlugins: 'link' }, | ||
| 'Link', | ||
| isDev ? /plugins\/link\/icons\/link\.png/gi : /plugins\/icons\.png/gi |
There was a problem hiding this comment.
All these assertions for standard icons differ only with plugin name. Maybe it could be simplified?
| @@ -0,0 +1,225 @@ | |||
| /* bender-tags: editor */ | |||
There was a problem hiding this comment.
There is no test for adding custom icon for standard buttons (e.g. Bold). It should be possible by overwriting button.icon property before rendering.
tests/plugins/button/buttonicon.js
Outdated
| /* bender-tags: editor */ | ||
| /* bender-ckeditor-plugins: button,toolbar */ | ||
|
|
||
| var editorCounter = 0, |
There was a problem hiding this comment.
Won't it be better to encapsulate such variables inside IIFE?
tests/plugins/button/buttonicon.js
Outdated
| ); | ||
| }, | ||
|
|
||
| 'test default button icon (hidpi)': function() { |
There was a problem hiding this comment.
Hm, personally I'd simplify tests and encapsulate switching between standard/hidpi inside createIconTest or something like that.
There was a problem hiding this comment.
This is problematic due to a fact how icons are registered. Icon is registered on plugin init so when plugin is loaded on standard resolution standard icons are initialized. Then switching to HiDPI and creating new editor (with same plugin) would not reregister the icons so the standard one is still in use.
This is the reason standard an HiDPI tests use different plugins. This is also a reason why generating two tests (standard one and HiDPI) from one editor config doesn't make sense thus auto generation doesn't make much sense (since you have to pass two different set of settings).
I refactored the tests creation to be more automated, however still standard and HiDPI tests are not generated form one set of settings.
f1ames
force-pushed
the
t/1530
branch
2 times, most recently
from
5b56ee3 to
d02bfcf
Compare
|
😭 Strangely, it should have standard (not HiDPI) set of icons... Probably for built version, setting manually |
|
The cause of failing tests is the way how tests are run with build version, check this comment for more details - b2d9f31#diff-6dfbc137c0c9a9951601ec7b03d6033aR17. |
|
Change sample icons color to red. Also I found one issue. After the bc3293f change, where
as class is based on |
|
Actually this is valid CSS class, that could be represented by escape sequence. What's more, it's also valid HTML class. E.g. So this change wasn't necessary, but at least we have nicer classes now ;) |
What is the purpose of this pull request?
New 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 possibility to use custom icons in buttons as described in #1530.
The only problem here was that there already had benn an undocumented
definition.iconproperty which was used to display icon defined by another plugin.This caused
definition.iconproperty to be more complex and docs get a little more complex too (well, there wasn't any before).Still, this behaviour has been preserved and plays nicely with the new one. I also added tests covering both behaviours.
Closes #1530.