fix (engine): code review fixes

ckeditor · Jul 25, 2023 · 32d4d8f · 32d4d8f
1 parent a753adc
commit 32d4d8f
Show file tree

Hide file tree

Showing 6 changed files with 22 additions and 17 deletions.
diff --git a/packages/ckeditor5-editor-balloon/src/ballooneditorui.ts b/packages/ckeditor5-editor-balloon/src/ballooneditorui.ts
@@ -117,7 +117,9 @@ export default class BalloonEditorUI extends EditorUI {
 			placeholderText = typeof placeholder === 'string' ? placeholder : placeholder[ editingRoot.rootName ];
 		}
 
-		editingRoot.placeholder = placeholderText;
+		if ( placeholderText ) {
+			editingRoot.placeholder = placeholderText;
+		}
 
 		enablePlaceholder( {
 			view: editingView,

diff --git a/packages/ckeditor5-editor-decoupled/src/decouplededitorui.ts b/packages/ckeditor5-editor-decoupled/src/decouplededitorui.ts
@@ -125,7 +125,9 @@ export default class DecoupledEditorUI extends EditorUI {
 			placeholderText = typeof placeholder === 'string' ? placeholder : placeholder[ editingRoot.rootName ];
 		}
 
-		editingRoot.placeholder = placeholderText;
+		if ( placeholderText ) {
+			editingRoot.placeholder = placeholderText;
+		}
 
 		enablePlaceholder( {
 			view: editingView,

diff --git a/packages/ckeditor5-editor-inline/src/inlineeditorui.ts b/packages/ckeditor5-editor-inline/src/inlineeditorui.ts
@@ -160,7 +160,9 @@ export default class InlineEditorUI extends EditorUI {
 			placeholderText = typeof placeholder === 'string' ? placeholder : placeholder[ editingRoot.rootName ];
 		}
 
-		editingRoot.placeholder = placeholderText;
+		if ( placeholderText ) {
+			editingRoot.placeholder = placeholderText;
+		}
 
 		if ( placeholderText ) {
 			enablePlaceholder( {

diff --git a/packages/ckeditor5-editor-multi-root/src/multirooteditorui.ts b/packages/ckeditor5-editor-multi-root/src/multirooteditorui.ts
@@ -207,7 +207,9 @@ export default class MultiRootEditorUI extends EditorUI {
 		const editingView = this.editor.editing.view;
 		const editingRoot = editingView.document.getRoot( editable.name! )!;
 
-		editingRoot.placeholder = placeholder;
+		if ( placeholder ) {
+			editingRoot.placeholder = placeholder;
+		}
 
 		enablePlaceholder( {
 			view: editingView,

diff --git a/packages/ckeditor5-engine/src/view/editableelement.ts b/packages/ckeditor5-engine/src/view/editableelement.ts
@@ -43,7 +43,9 @@ export default class EditableElement extends ObservableMixin( ContainerElement )
 	declare public isFocused: boolean;
 
 	/**
-	 * Placeholder of current editable element.
+	 * Placeholder of editable element. Could be updated.
+	 *
+	 * editor.editing.view.document.getRoot( 'main' ).placeholder = 'New placeholder';
 	 *
 	 * @observable
 	 */

diff --git a/packages/ckeditor5-engine/src/view/placeholder.ts b/packages/ckeditor5-engine/src/view/placeholder.ts
@@ -31,7 +31,6 @@ const documentPlaceholders = new WeakMap<Document, Map<Element, PlaceholderConfi
  * @param options Configuration options of the placeholder.
  * @param options.view Editing view instance.
  * @param options.element Element that will gain a placeholder. See `options.isDirectHost` to learn more.
- * @param options.text Placeholder text.
  * @param options.isDirectHost If set `false`, the placeholder will not be enabled directly
  * in the passed `element` but in one of its children (selected automatically, i.e. a first empty child element).
  * Useful when attaching placeholders to elements that can host other elements (not just text), for instance,
@@ -62,22 +61,18 @@ export function enablePlaceholder( { view, element, isDirectHost = true, keepOnF
 
 	if ( element.is( 'editableElement' ) ) {
 		element.on( 'change:placeholder', ( evtInfo, evt, text ) => {
-			// Store information about the element placeholder under its document.
-			documentPlaceholders.get( doc )!.set( element, {
-				text,
-				isDirectHost,
-				keepOnFocus,
-				hostElement: isDirectHost ? element : null
-			} );
-
-			// Update the placeholders right away.
-			view.change( writer => updateDocumentPlaceholders( doc, writer ) );
+			setPlaceholder( text );
 		} );
 	}
 
 	if ( element.placeholder ) {
+		setPlaceholder( element.placeholder );
+	}
+
+	function setPlaceholder( text: string ) {
+		// Store information about the element placeholder under its document.
 		documentPlaceholders.get( doc )!.set( element, {
-			text: element.placeholder,
+			text,
 			isDirectHost,
 			keepOnFocus,
 			hostElement: isDirectHost ? element : null