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

Update Placeholder after initialization #14627

Merged
merged 15 commits into from
Jul 26, 2023

Conversation

illia-stv
Copy link
Contributor

@illia-stv illia-stv commented Jul 20, 2023

Suggested merge commit message (convention)

MINOR BREAKING CHANGE (engine): placeholder could be be updated after initialization. Closes #9925.


Additional information

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

@illia-stv illia-stv self-assigned this Jul 20, 2023
@Dumluregn Dumluregn requested review from oleq and removed request for oleq July 21, 2023 09:41
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 remarks, there was also no breaking change information - be sure to add it as this is a backward-incompatible change.

Also, I don't see a PR in collaboration features repo. You must also add a related PR in there as at least commenteditor.ts file uses the enablePlaceholder function.

@illia-stv illia-stv requested a review from mlewand July 25, 2023 12:38
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.

The following docs are no longer true:

* To change the placeholder text, simply call this method again with new options.

You should update it so that it describes how one should change the placeholder from here on.


* Enable the placeholder text on the editing root, if any was configured.

Be sure to check the surrounding API docs when changing something.

Enable the placeholder text on the editing root, if any was configured.

"if any was configured" part is no longer true after your changes. Because placeholder mechanism is added anyway and it listens to the placeholder property.


Logic like that:

private _initPlaceholder(): void {
const editor = this.editor;
const editingView = editor.editing.view;
const editingRoot = editingView.document.getRoot()!;
const placeholder = editor.config.get( 'placeholder' );
if ( placeholder ) {
const placeholderText = typeof placeholder === 'string' ? placeholder : placeholder[ editingRoot.rootName ];
if ( placeholderText ) {
editingRoot.placeholder = placeholderText;
}
}
enablePlaceholder( {
view: editingView,
element: editingRoot,
isDirectHost: false,
keepOnFocus: true
} );
}

Is repeated in multiple places. It would be a good idea to make a common helper reused by all the editor UIs.

docs/features/editor-placeholder.md Outdated Show resolved Hide resolved
docs/features/editor-placeholder.md Outdated Show resolved Hide resolved
docs/features/editor-placeholder.md Outdated Show resolved Hide resolved
packages/ckeditor5-editor-balloon/src/ballooneditorui.ts Outdated Show resolved Hide resolved
packages/ckeditor5-editor-inline/src/inlineeditorui.ts Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/view/editableelement.ts Outdated Show resolved Hide resolved
@mlewand
Copy link
Contributor

mlewand commented Jul 26, 2023

I'll handle the review comments to conclude this PR.

@mlewand mlewand merged commit a7e0947 into master Jul 26, 2023
@mlewand mlewand deleted the ck/9925-update-placeholder-after-initialization branch July 26, 2023 13:55
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.

Update Placeholder after initialization
2 participants