-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fix for: Custom colors added to colorButton_colors
in form label/code don't work correctly
#2646
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
<script> | ||
CKEDITOR.replace( 'editor', { | ||
colorButton_colors: 'Not blue/F00,00F' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe restrict toolbar to only one colorbutton? ATM there are two buttons: for background and text.
plugins/colorbutton/plugin.js
Outdated
@@ -283,8 +282,8 @@ CKEDITOR.plugins.add( 'colorbutton', { | |||
output.push( '<td>' + | |||
'<a class="cke_colorbox" _cke_focus=1 hidefocus=true' + | |||
' title="', colorLabel, '"' + | |||
' onclick="CKEDITOR.tools.callFunction(', clickFn, ',\'', colorName, '\',\'', type, '\'); return false;"' + | |||
' href="javascript:void(\'', colorLabel, '\')"' + | |||
' onclick="CKEDITOR.tools.callFunction(', clickFn, ',\'#', colorCode, '\',\'', type, '\'); return false;"' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO normalization of the color should be done inside clickFn
.
CHANGES.md
Outdated
@@ -6,6 +6,7 @@ | |||
Fixed Issues: | |||
|
|||
* [#1986](https://github.com/ckeditor/ckeditor-dev/issues/1986): Fixed: Cell Properties dialog from [Table Tools](https://ckeditor.com/cke4/addon/tabletools) plugin shows styles that are not allowed through [`config.allowedContent`](https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_config.html#cfg-allowedContent). | |||
* [#1478](https://github.com/ckeditor/ckeditor-dev/issues/1478): Fixed: Custom colors added to [Color Button](https://ckeditor.com/cke4/addon/colorbutton) via `colorButton_colors` in form label/code don't work correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Config variable can be linked.
bb73fbb
to
5b7d1c1
Compare
I've added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact it's a breaking change in behaviour and should be noted in API docs. Also the whole docs should be updated.
709e7b3
to
4d0a7da
Compare
Updated docs, and rebased branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rebase branch onto latest major
.
4d0a7da
to
aa7429c
Compare
Rebased, updated version references. |
…on't work correctly.
…sn't affect color set by color dialog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
What is the purpose of this pull request?
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?
Forced plugin to set
colorCode
instead ofcolorName
on text.Reason behind this change can be found here: #1478 (comment)
Closes #1478