Fix for placeholder ck class removal
#1391
Conversation
|
Unless someone's against, https://github.com/ckeditor/ckeditor5-image/issues/198#issuecomment-378211139 contains a solution which differs from the one proposes in this PR so I'm R-ing it. |
src/view/placeholder.js
Outdated
| @@ -48,13 +55,22 @@ export function attachPlaceholder( view, element, placeholderText, checkFunction | |||
| */ | |||
| export function detachPlaceholder( view, element ) { | |||
| const document = element.document; | |||
| let removeCkClass = false; | |||
There was a problem hiding this comment.
Why is this variable defined here? Why can't view.change() be in the if()?
src/view/placeholder.js
Outdated
|
|
||
| // Store information if 'ck' class is already present and don't remove it when detaching placeholder. | ||
| // https://github.com/ckeditor/ckeditor5-image/issues/198#issuecomment-377542222 | ||
| removeCkClass: !element.hasClass( 'ck' ) |
There was a problem hiding this comment.
Whether to remove the ck class when detaching the placeholder.
There was a problem hiding this comment.
"if 'ck' class is already present" is exactly what the code below is so it's unnecessary to additionally write this in a comment.
| @@ -105,8 +121,8 @@ function updateSinglePlaceholder( writer, element, info ) { | |||
|
|
|||
| // If checkFunction is provided and returns false - remove placeholder. | |||
| if ( checkFunction && !checkFunction() ) { | |||
| if ( element.hasClass( 'ck', 'ck-placeholder' ) ) { | |||
| writer.removeClass( [ 'ck', 'ck-placeholder' ], element ); | |||
| if ( element.hasClass( 'ck-placeholder' ) ) { | |||
There was a problem hiding this comment.
I like that you decided to not remove .ck when the placeholder is not needed. It's simpler and totally reasonable – the .ck-placeholder class is responsible for showing the thing. The .ck class makes for the base.
src/view/placeholder.js
Outdated
| if ( info.removeCkClass ) { | ||
| view.change( writer => writer.removeClass( 'ck', element ) ); | ||
| } | ||
|
|
||
| documentPlaceholders.get( document ).delete( element ); | ||
| } | ||
|
|
||
| view.change( writer => { |
There was a problem hiding this comment.
We have two view.change() calls which mean rendering the thing twice. Can't we have one big view.change()? It doesn't actually matter, but I'm asking out of curiosity.
|
We have some doubts whether using the |
|
Followup in ckeditor/ckeditor5#936. |
Suggested merge commit message (convention)
Fix: Placeholder is removing
ckCSS class only if it was added by that placeholder. Closes https://github.com/ckeditor/ckeditor5-image/issues/198.