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

Conversation

illia-stv
Copy link
Contributor

Suggested merge commit message (convention)

Fix (engine): enablePlaceholder() API remain backward compatible for some time. Closes #14743.


Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

@illia-stv illia-stv self-assigned this Aug 4, 2023
Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

Couple of adjustments ad we're good to go 👍

Be sure to check tests with --production CLI flag for testing as it might forbid using window.console.

packages/ckeditor5-engine/src/view/placeholder.ts Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/view/placeholder.ts Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/view/placeholder.ts Outdated Show resolved Hide resolved
packages/ckeditor5-engine/tests/view/placeholder.js Outdated Show resolved Hide resolved
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

@mlewand
Copy link
Contributor

mlewand commented Aug 7, 2023

And my assumption was correct, the test are failing on CI: https://app.travis-ci.com/github/ckeditor/ckeditor5/jobs/607359829

@illia-stv illia-stv requested a review from mlewand August 7, 2023 11:30
@illia-stv
Copy link
Contributor Author

illia-stv commented Aug 7, 2023

@mlewand There is one additional suggestion by @Dumluregn to perform displaying warning by logWarning.

image

Is worth to mention when you open your dev console you will see only error name with link to the documentation.

image

image

What do you think?

@mlewand
Copy link
Contributor

mlewand commented Aug 7, 2023

@mlewand There is one additional suggestion by @Dumluregn to perform displaying warning by logWarning.

(...)

Yes, it makes sense @illia-stv! I missed that one.

);
/**
* 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, this new description is missing link to the migration guide that you provided below. That's very helpful so it needs to be here.

I'll add it as a part of R.

@mlewand mlewand self-requested a review August 7, 2023 14:43
Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

Added some minor docs tweaks, LGTM.

@mlewand mlewand merged commit 8936e0e into master Aug 7, 2023
@mlewand mlewand deleted the ck/14743-enablePlaceholder-API-should-remain-backward-compatible-for-some-time branch August 7, 2023 15:28
pomek pushed a commit that referenced this pull request Aug 8, 2023
…I-should-remain-backward-compatible-for-some-time

Fix (engine): Made the `enablePlaceholder()` API to remain backward compatible for the deprecation period. It will be removed in the future. Closes #14743.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enablePlaceholder() API should remain backward compatible for some time
2 participants