Skip to content
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

enablePlaceholder() API should remain backward compatible for some time #14753

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 33 additions & 2 deletions packages/ckeditor5-engine/src/view/placeholder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,13 @@ import type EditableElement from './editableelement';
import type Element from './element';
import type View from './view';

import type { ObservableChangeEvent } from '@ckeditor/ckeditor5-utils';
import { logWarning, type ObservableChangeEvent } from '@ckeditor/ckeditor5-utils';

// Each document stores information about its placeholder elements and check functions.
const documentPlaceholders = new WeakMap<Document, Map<Element, PlaceholderConfig>>();

let hasDisplayedPlaceholderDeprecationWarning = false;

/**
* A helper that enables a placeholder on the provided view element (also updates its visibility).
* The placeholder is a CSS pseudo–element (with a text content) attached to the element.
Expand All @@ -35,12 +37,15 @@ const documentPlaceholders = new WeakMap<Document, Map<Element, PlaceholderConfi
* 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,
* editable root elements.
* @param options.text Placeholder text. It's **deprecated** and will be removed soon. Use
* {@link module:engine/view/placeholder~PlaceholderableElement#placeholder `options.element.placeholder`} instead.
* @param options.keepOnFocus If set `true`, the placeholder stay visible when the host element is focused.
*/
export function enablePlaceholder( { view, element, isDirectHost = true, keepOnFocus = false }: {
export function enablePlaceholder( { view, element, text, isDirectHost = true, keepOnFocus = false }: {
illia-stv marked this conversation as resolved.
Show resolved Hide resolved
view: View;
element: PlaceholderableElement | EditableElement;
isDirectHost?: boolean;
text?: string;
keepOnFocus?: boolean;
} ): void {
const doc = view.document;
Expand All @@ -67,6 +72,12 @@ export function enablePlaceholder( { view, element, isDirectHost = true, keepOnF

if ( element.placeholder ) {
setPlaceholder( element.placeholder );
} else if ( text ) {
setPlaceholder( text );
}

if ( text ) {
mlewand marked this conversation as resolved.
Show resolved Hide resolved
showPlaceholderTextDeprecationWarning();
}

function setPlaceholder( text: string ) {
Expand Down Expand Up @@ -299,6 +310,26 @@ function getChildPlaceholderHostSubstitute( parent: Element ): Element | null {
return null;
}

/**
* Displays a deprecation warning message in the console, but only once per page load.
*/
function showPlaceholderTextDeprecationWarning() {
if ( !hasDisplayedPlaceholderDeprecationWarning ) {
/**
* The "text" option in the {@link module:engine/view/placeholder~enablePlaceholder `enablePlaceholder()`}
* function is deprecated and will be removed soon.
*
* See the {@glink updating/guides/update-to-39#view-element-placeholder Migration to v39} guide for
* more information on how to apply this change.
*
* @error enableplaceholder-deprecated-text-option
*/
logWarning( 'enableplaceholder-deprecated-text-option' );
}

hasDisplayedPlaceholderDeprecationWarning = true;
}

/**
* Configuration of the placeholder.
*/
Expand Down
55 changes: 52 additions & 3 deletions packages/ckeditor5-engine/tests/view/placeholder.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
*/

/* globals console */

import {
enablePlaceholder,
disablePlaceholder,
Expand Down Expand Up @@ -618,8 +620,7 @@ describe( 'placeholder', () => {

enablePlaceholder( {
view,
element: viewRoot,
text: 'foo bar baz'
element: viewRoot
} );
viewRoot.placeholder = 'new placeholder';

Expand All @@ -632,12 +633,60 @@ describe( 'placeholder', () => {
enablePlaceholder( {
view,
element: viewRoot,
text: 'foo bar baz',
isDirectHost: false
} );
viewRoot.placeholder = 'new placeholder';

expect( viewRoot.getChild( 0 ).getAttribute( 'data-placeholder' ) ).to.equal( 'new placeholder' );
} );

it( 'should through warning once if "text" is used as argument', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What did you mean by "should through warning once"?

Copy link
Contributor Author

@illia-stv illia-stv Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning should be through only once in dev console

sinon.stub( console, 'warn' );

setData( view, '<div></div><div>{another div}</div>' );

enablePlaceholder( {
view,
element: viewRoot,
isDirectHost: false,
text: 'foo bar'
} );

enablePlaceholder( {
view,
element: viewRoot,
isDirectHost: false,
text: 'foo bar baz'
} );

sinon.assert.calledOnce( console.warn );
expect( console.warn.calledWith( sinon.match( /^enableplaceholder-deprecated-text-option/ ) ) ).to.be.true;
} );

it( 'should set placeholder using "text" argument', () => {
setData( view, '<div></div><div>{another div}</div>' );

enablePlaceholder( {
view,
element: viewRoot,
text: 'placeholder'
} );

expect( viewRoot.getAttribute( 'data-placeholder' ) ).to.equal( 'placeholder' );
} );

it( 'should prefer element\'s placeholder value over text parameter', () => {
setData( view, '<div></div><div>{another div}</div>' );

enablePlaceholder( {
view,
element: viewRoot,
text: 'foo'
} );

viewRoot.placeholder = 'bar';

expect( viewRoot.getAttribute( 'data-placeholder' ) ).to.equal( 'bar' );
} );
} );
} );