From f02b2b456f229f950801b76a3daa4df78231932d Mon Sep 17 00:00:00 2001 From: Illia Sheremetov Date: Fri, 4 Aug 2023 13:27:50 +0200 Subject: [PATCH 1/8] Fix (engine): back backward compatibilty --- .../ckeditor5-engine/src/view/placeholder.ts | 24 +++++++- .../tests/view/placeholder.js | 57 ++++++++++++++++++- 2 files changed, 77 insertions(+), 4 deletions(-) diff --git a/packages/ckeditor5-engine/src/view/placeholder.ts b/packages/ckeditor5-engine/src/view/placeholder.ts index 0e374d08f48..931f2b3e7b3 100644 --- a/packages/ckeditor5-engine/src/view/placeholder.ts +++ b/packages/ckeditor5-engine/src/view/placeholder.ts @@ -20,6 +20,8 @@ import type { ObservableChangeEvent } from '@ckeditor/ckeditor5-utils'; // Each document stores information about its placeholder elements and check functions. const documentPlaceholders = new WeakMap>(); +let hasDisplayedWarning = 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. @@ -37,10 +39,11 @@ const documentPlaceholders = new WeakMap { enablePlaceholder( { view, - element: viewRoot, - text: 'foo bar baz' + element: viewRoot } ); viewRoot.placeholder = 'new placeholder'; @@ -632,12 +633,62 @@ 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', () => { + const spy = sinon.spy( console, 'warn' ); + const warning = 'Deprecation Warning: The "text" argument in the "enableProperty" function will be deprecated ' + + 'in the next version. Please update your code. Refer to the documentation for alternative usage: ' + + 'https://ckeditor.com/docs/ckeditor5/latest/updating/guides/update-to-39.html#view-element-placeholder'; + + setData( view, '
{another div}
' ); + + enablePlaceholder( { + view, + element: viewRoot, + isDirectHost: false, + text: 'foo bar' + } ); + + enablePlaceholder( { + view, + element: viewRoot, + isDirectHost: false, + text: 'foo bar baz' + } ); + + sinon.assert.calledOnce( spy.withArgs( warning ) ); + } ); + + it( 'should set placeholder using "text" argument', () => { + setData( view, '
{another div}
' ); + + enablePlaceholder( { + view, + element: viewRoot, + text: 'placeholder' + } ); + + expect( viewRoot.getAttribute( 'data-placeholder' ) ).to.equal( 'placeholder' ); + } ); + + it( 'should use element\'s placeholder as more prior value', () => { + setData( view, '
{another div}
' ); + + enablePlaceholder( { + view, + element: viewRoot, + text: 'foo' + } ); + + viewRoot.placeholder = 'bar'; + + expect( viewRoot.getAttribute( 'data-placeholder' ) ).to.equal( 'bar' ); + } ); } ); } ); From 6ae434dfd18f10508e7ffd2c5034768aa800b847 Mon Sep 17 00:00:00 2001 From: Illia Sheremetov Date: Mon, 7 Aug 2023 11:26:40 +0200 Subject: [PATCH 2/8] Fix (engine): extend test --- packages/ckeditor5-engine/tests/view/placeholder.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/ckeditor5-engine/tests/view/placeholder.js b/packages/ckeditor5-engine/tests/view/placeholder.js index 0ba47fe6e52..4b80cb07f57 100644 --- a/packages/ckeditor5-engine/tests/view/placeholder.js +++ b/packages/ckeditor5-engine/tests/view/placeholder.js @@ -662,6 +662,7 @@ describe( 'placeholder', () => { text: 'foo bar baz' } ); + sinon.assert.calledOnce( console.warn ); sinon.assert.calledOnce( spy.withArgs( warning ) ); } ); From 25f5b5a660c8cf20cab267391352a29fef230b21 Mon Sep 17 00:00:00 2001 From: Illia Sheremetov Date: Mon, 7 Aug 2023 12:58:09 +0200 Subject: [PATCH 3/8] Fix (engine): code review fixes --- .../ckeditor5-engine/src/view/placeholder.ts | 19 +++++++++++-------- .../tests/view/placeholder.js | 6 +++--- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/packages/ckeditor5-engine/src/view/placeholder.ts b/packages/ckeditor5-engine/src/view/placeholder.ts index 931f2b3e7b3..7d1f6eb8c31 100644 --- a/packages/ckeditor5-engine/src/view/placeholder.ts +++ b/packages/ckeditor5-engine/src/view/placeholder.ts @@ -20,7 +20,7 @@ import type { ObservableChangeEvent } from '@ckeditor/ckeditor5-utils'; // Each document stores information about its placeholder elements and check functions. const documentPlaceholders = new WeakMap>(); -let hasDisplayedWarning = false; +let hasDisplayedPlaceholderDeprecationWarning = false; /** * A helper that enables a placeholder on the provided view element (also updates its visibility). @@ -37,6 +37,7 @@ let hasDisplayedWarning = false; * 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. Is deprecated and will be removed soon. * @param options.keepOnFocus If set `true`, the placeholder stay visible when the host element is focused. */ export function enablePlaceholder( { view, element, text, isDirectHost = true, keepOnFocus = false }: { @@ -75,7 +76,7 @@ export function enablePlaceholder( { view, element, text, isDirectHost = true, k } if ( text ) { - showDeprecationWarning(); + showPlaceholderTextDeprecationWarning(); } function setPlaceholder( text: string ) { @@ -311,14 +312,16 @@ function getChildPlaceholderHostSubstitute( parent: Element ): Element | null { /** * Displays a deprecation warning message in the console, but only once per page load. */ -function showDeprecationWarning() { - if ( !hasDisplayedWarning ) { - console.warn( 'Deprecation Warning: The "text" argument in the "enableProperty" function will be deprecated ' + - 'in the next version. Please update your code. Refer to the documentation for alternative usage: ' + - 'https://ckeditor.com/docs/ckeditor5/latest/updating/guides/update-to-39.html#view-element-placeholder' ); +function showPlaceholderTextDeprecationWarning() { + if ( !hasDisplayedPlaceholderDeprecationWarning ) { + console.warn( + 'Deprecation Warning: The "text" argument in the "enableProperty" function is already deprecated ' + + 'and will be removed soon. Please update your code. Refer to the documentation for alternative usage: ' + + 'https://ckeditor.com/docs/ckeditor5/latest/updating/guides/update-to-39.html#view-element-placeholder' + ); } - hasDisplayedWarning = true; + hasDisplayedPlaceholderDeprecationWarning = true; } /** diff --git a/packages/ckeditor5-engine/tests/view/placeholder.js b/packages/ckeditor5-engine/tests/view/placeholder.js index 4b80cb07f57..4869b96225b 100644 --- a/packages/ckeditor5-engine/tests/view/placeholder.js +++ b/packages/ckeditor5-engine/tests/view/placeholder.js @@ -641,7 +641,7 @@ describe( 'placeholder', () => { } ); it( 'should through warning once if "text" is used as argument', () => { - const spy = sinon.spy( console, 'warn' ); + sinon.stub( console, 'warn' ); const warning = 'Deprecation Warning: The "text" argument in the "enableProperty" function will be deprecated ' + 'in the next version. Please update your code. Refer to the documentation for alternative usage: ' + 'https://ckeditor.com/docs/ckeditor5/latest/updating/guides/update-to-39.html#view-element-placeholder'; @@ -663,7 +663,7 @@ describe( 'placeholder', () => { } ); sinon.assert.calledOnce( console.warn ); - sinon.assert.calledOnce( spy.withArgs( warning ) ); + sinon.assert.calledWithExactly( console.warn, warning ); } ); it( 'should set placeholder using "text" argument', () => { @@ -678,7 +678,7 @@ describe( 'placeholder', () => { expect( viewRoot.getAttribute( 'data-placeholder' ) ).to.equal( 'placeholder' ); } ); - it( 'should use element\'s placeholder as more prior value', () => { + it( 'should prefer element\'s placeholder value over text parameter', () => { setData( view, '
{another div}
' ); enablePlaceholder( { From b08c2bd18e57b2f3a3e756fe81097cb08977396a Mon Sep 17 00:00:00 2001 From: Illia Sheremetov Date: Mon, 7 Aug 2023 13:00:02 +0200 Subject: [PATCH 4/8] Test (engine): fix test --- packages/ckeditor5-engine/tests/view/placeholder.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-engine/tests/view/placeholder.js b/packages/ckeditor5-engine/tests/view/placeholder.js index 4869b96225b..c305e145e25 100644 --- a/packages/ckeditor5-engine/tests/view/placeholder.js +++ b/packages/ckeditor5-engine/tests/view/placeholder.js @@ -642,8 +642,8 @@ describe( 'placeholder', () => { it( 'should through warning once if "text" is used as argument', () => { sinon.stub( console, 'warn' ); - const warning = 'Deprecation Warning: The "text" argument in the "enableProperty" function will be deprecated ' + - 'in the next version. Please update your code. Refer to the documentation for alternative usage: ' + + const warning = 'Deprecation Warning: The "text" argument in the "enableProperty" function is already deprecated ' + + 'and will be removed soon. Please update your code. Refer to the documentation for alternative usage: ' + 'https://ckeditor.com/docs/ckeditor5/latest/updating/guides/update-to-39.html#view-element-placeholder'; setData( view, '
{another div}
' ); From eb895201a906410a231d7a814993b643a37cb090 Mon Sep 17 00:00:00 2001 From: Illia Sheremetov Date: Mon, 7 Aug 2023 15:26:28 +0200 Subject: [PATCH 5/8] Fix (engine): perform displaying warning with 'logWarning' --- packages/ckeditor5-engine/src/view/placeholder.ts | 14 ++++++++------ .../ckeditor5-engine/tests/view/placeholder.js | 5 +---- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/packages/ckeditor5-engine/src/view/placeholder.ts b/packages/ckeditor5-engine/src/view/placeholder.ts index 7d1f6eb8c31..f45eab1e05a 100644 --- a/packages/ckeditor5-engine/src/view/placeholder.ts +++ b/packages/ckeditor5-engine/src/view/placeholder.ts @@ -15,7 +15,7 @@ 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>(); @@ -314,11 +314,13 @@ function getChildPlaceholderHostSubstitute( parent: Element ): Element | null { */ function showPlaceholderTextDeprecationWarning() { if ( !hasDisplayedPlaceholderDeprecationWarning ) { - console.warn( - 'Deprecation Warning: The "text" argument in the "enableProperty" function is already deprecated ' + - 'and will be removed soon. Please update your code. Refer to the documentation for alternative usage: ' + - 'https://ckeditor.com/docs/ckeditor5/latest/updating/guides/update-to-39.html#view-element-placeholder' - ); + /** + * The "text" option in the {@link module:engine/view/placeholder~enablePlaceholder `enablePlaceholder()`} + * function is already deprecated and will be removed soon. Please update your code. + * + * @error enableplaceholder-deprecated-text-option + */ + logWarning( 'enableplaceholder-deprecated-text-option' ); } hasDisplayedPlaceholderDeprecationWarning = true; diff --git a/packages/ckeditor5-engine/tests/view/placeholder.js b/packages/ckeditor5-engine/tests/view/placeholder.js index c305e145e25..d520d8793ce 100644 --- a/packages/ckeditor5-engine/tests/view/placeholder.js +++ b/packages/ckeditor5-engine/tests/view/placeholder.js @@ -642,9 +642,6 @@ describe( 'placeholder', () => { it( 'should through warning once if "text" is used as argument', () => { sinon.stub( console, 'warn' ); - const warning = 'Deprecation Warning: The "text" argument in the "enableProperty" function is already deprecated ' + - 'and will be removed soon. Please update your code. Refer to the documentation for alternative usage: ' + - 'https://ckeditor.com/docs/ckeditor5/latest/updating/guides/update-to-39.html#view-element-placeholder'; setData( view, '
{another div}
' ); @@ -663,7 +660,7 @@ describe( 'placeholder', () => { } ); sinon.assert.calledOnce( console.warn ); - sinon.assert.calledWithExactly( console.warn, warning ); + expect( console.warn.calledWith( sinon.match( /^enableplaceholder-deprecated-text-option/ ) ) ).to.be.true; } ); it( 'should set placeholder using "text" argument', () => { From f0f567d12b63965bd2f1d60373a223744591fe2e Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Mon, 7 Aug 2023 16:20:48 +0200 Subject: [PATCH 6/8] Minor adjustments in docs. --- packages/ckeditor5-engine/src/view/placeholder.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-engine/src/view/placeholder.ts b/packages/ckeditor5-engine/src/view/placeholder.ts index f45eab1e05a..67e8a0ead44 100644 --- a/packages/ckeditor5-engine/src/view/placeholder.ts +++ b/packages/ckeditor5-engine/src/view/placeholder.ts @@ -37,7 +37,8 @@ let hasDisplayedPlaceholderDeprecationWarning = false; * 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. Is deprecated and will be removed soon. + * @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, text, isDirectHost = true, keepOnFocus = false }: { @@ -316,7 +317,10 @@ function showPlaceholderTextDeprecationWarning() { if ( !hasDisplayedPlaceholderDeprecationWarning ) { /** * The "text" option in the {@link module:engine/view/placeholder~enablePlaceholder `enablePlaceholder()`} - * function is already deprecated and will be removed soon. Please update your code. + * function is deprecated and will be removed soon. + * + * See {@link https://ckeditor.com/docs/ckeditor5/latest/updating/guides/update-to-39.html#view-element-placeholder migration guide} + * for more information on how to apply this change. * * @error enableplaceholder-deprecated-text-option */ From c0645159fca56944f067a915a9f5c30d39258d9c Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Mon, 7 Aug 2023 16:42:21 +0200 Subject: [PATCH 7/8] Fixed migration guide link in API docs. --- packages/ckeditor5-engine/src/view/placeholder.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/ckeditor5-engine/src/view/placeholder.ts b/packages/ckeditor5-engine/src/view/placeholder.ts index 67e8a0ead44..2d592b785e0 100644 --- a/packages/ckeditor5-engine/src/view/placeholder.ts +++ b/packages/ckeditor5-engine/src/view/placeholder.ts @@ -319,8 +319,7 @@ function showPlaceholderTextDeprecationWarning() { * The "text" option in the {@link module:engine/view/placeholder~enablePlaceholder `enablePlaceholder()`} * function is deprecated and will be removed soon. * - * See {@link https://ckeditor.com/docs/ckeditor5/latest/updating/guides/update-to-39.html#view-element-placeholder migration guide} - * for more information on how to apply this change. + * See the {@glink updating/guides/update-to-39 Migration to v39} guide for more information on how to apply this change. * * @error enableplaceholder-deprecated-text-option */ From 3c9cf29c736893f7db871fac5fa420f57f3058c7 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Mon, 7 Aug 2023 16:57:23 +0200 Subject: [PATCH 8/8] Use even more precise link in API docs. --- packages/ckeditor5-engine/src/view/placeholder.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-engine/src/view/placeholder.ts b/packages/ckeditor5-engine/src/view/placeholder.ts index 2d592b785e0..fbad1525bd5 100644 --- a/packages/ckeditor5-engine/src/view/placeholder.ts +++ b/packages/ckeditor5-engine/src/view/placeholder.ts @@ -319,7 +319,8 @@ function showPlaceholderTextDeprecationWarning() { * 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 Migration to v39} guide for more information on how to apply this change. + * 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 */