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

#8871: Integrate inline images with the link feature #9438

Merged
merged 46 commits into from
Apr 20, 2021

Conversation

oleq
Copy link
Member

@oleq oleq commented Apr 8, 2021

Suggested merge commit message (convention)

Other (link): Integrated the link feature with inline images. Closes #8871. Closes #9017. Closes #9167.

Other (image): Turned the image utils module into an editor plugin to allow sharing utils outside the package. Code refactoring (see #8871).


Additional information

BREAKING CHANGE (image): The image utils module (@ckeditor/ckeditor5-image/src/image/utils.js) has turned into the ImageUtils plugin. The helpers are still accessible via editor.plugins.get( 'ImageUtils' ) namespace, for instance, editor.plugins.get( 'ImageUtils' ).isImage( ... ).

BREAKING CHANGE (image): The linked image indicator (icon) rendered as a <span> with the .ck-link-image_icon CSS class has been removed. To alter the look of the indicator (including the icon), please use figure.image > a::after (for linked block images) and a span.image-inline::after (for linked inline images) CSS selectors instead.

BREAKING CHANGE (image): The API of the insertImage() helper has changed, please make sure to update your integrations.

BREAKING CHANGE (image): The isImageWidget() helper is no longer exposed by the Image plugin. Use the ImageUtils plugin to access this helper instead.

BREAKING CHANGE (image): The API of common image converters has changed. Please refer to the documentation to learn more.

BREAKING CHANGE (image): The API of various image caption utils has changed. Please refer to the documentation to learn more.

@oleq oleq requested a review from niegowski April 15, 2021 09:13
@oleq oleq marked this pull request as ready for review April 15, 2021 09:13
Copy link
Contributor

@niegowski niegowski left a comment

Choose a reason for hiding this comment

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

The image linking looks good but I really do not like that we put all the helpers into ImageUtils. I'd expect it to expose only the helpers used in the other package.

packages/ckeditor5-image/src/image/utils.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/src/image/utils.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/src/image/utils.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/src/image/utils.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/src/image/utils.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/src/image/utils.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/src/image/utils.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/src/autoimage.js Show resolved Hide resolved
packages/ckeditor5-link/src/linkimageediting.js Outdated Show resolved Hide resolved
@oleq oleq requested a review from niegowski April 19, 2021 14:57
Copy link
Contributor

@niegowski niegowski left a comment

Choose a reason for hiding this comment

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

Some comments as below. Tests not reviewed yet.

packages/ckeditor5-image/src/autoimage.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/src/image/imageblockediting.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/src/image/imageinlineediting.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/src/image/utils.js Outdated Show resolved Hide resolved
@oleq oleq requested a review from niegowski April 20, 2021 07:21
@oleq
Copy link
Member Author

oleq commented Apr 20, 2021

There are plenty of conflicts now since #9459 was merged and I'm addressing them.

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.

2 participants