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

i/7794: Introduce Insert Image via URL feature. #7835

Merged
merged 40 commits into from
Aug 20, 2020
Merged

i/7794: Introduce Insert Image via URL feature. #7835

merged 40 commits into from
Aug 20, 2020

Conversation

panr
Copy link
Contributor

@panr panr commented Aug 13, 2020

Suggested merge commit message (convention)

Feature (image): Introduced insert image via URL feature. Closes #7794.

Other (image): Update ImageUploadUI plugin. From now the plugin appears as a dropdown with the split button. Primary action shows the native dialog for picking the images from the disk, and the arrow button opens the dropdown with the form for inserting an image via URL and configured integrations.

BREAKING CHANGE (image): Keep the ImageUploadCommand enabled when an image in the editor's content is selected. This allows us to replace the source URL of the image without the need to remove it.

@pomek pomek self-assigned this Aug 17, 2020
Copy link
Member

@pomek pomek left a comment

Choose a reason for hiding this comment

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

As in the review, the most important case here is the API for the ImageUploadPanelView class.

packages/ckeditor5-image/src/imageupload/imageuploadui.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/src/imageupload/imageuploadui.js Outdated Show resolved Hide resolved
packages/ckeditor5-image/src/imageupload/imageuploadui.js Outdated Show resolved Hide resolved
* @param {Object} [options.integrations={insertImageViaUrl:'insertImageViaUrl'}] Integrations object that contain
* components (or tokens for components) to be shown in the panel view. By default it has `insertImageViaUrl` view.
*/
constructor( locale, options = { integrations: { insertImageViaUrl: 'insertImageViaUrl' } } ) {
Copy link
Member

Choose a reason for hiding this comment

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

TL;DR: integrations should be the second parameter. It should be an array that keeps View instances.


And short "ADR" how I figured it out.

I would change the API a little bit:

Suggested change
constructor( locale, options = { integrations: { insertImageViaUrl: 'insertImageViaUrl' } } ) {
constructor( locale, integrations ) {

And integrations will be an array now. Items of the array could be:

[
    // Passing a `View` instance that renders the component.
    { name: 'name-for-integration-01', view: new CustomIntegrationView( /* ... */ ),

    // Passing a name of the View that is provided by the class by default.
    { name: 'integration-02', view: 'insertImageViaUrl'},

    // It won't happen directly since `prepareIntegrations()` function will filter out the invalid value. However, the class itself does not check what is being added.
    // We could assume that view will be always an instance of the `View` class.
    { name: 'integration-03', view: 'non-existing-item'},
]

The order of items in the array could determine the order in the Image Upload Panel view. Hence, the name key could be omitted and we could pass only View instances or the default string.

And at this point, I don't see any sense to support the string value here. From the ImageUploadPanelView class point of view, integrations should represent a collection (the array) of View elements.

Preparing proper values/view instances should be done a layer earlier.

Copy link
Member

Choose a reason for hiding this comment

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

Since the proposed (the comment above) changes will cause changes in the class itself, I am skipping reviewing this file and its tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should ask others. But ATM it looks like a followup.

packages/ckeditor5-image/src/imageupload/utils.js Outdated Show resolved Hide resolved
* @private
* @returns {module:ui/labeledfield/labeledfieldview~LabeledFieldView}
*/
_createLabeledInputView( locale ) {
Copy link
Member

Choose a reason for hiding this comment

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

It's sad that you can type whatever in the input and it won't be checked. I am unsure of what could we do here. Check whether passed a relative path or an absolute to the resource and validate the later one? I am thinking about the AutoLink regexp.

Copy link
Member

Choose a reason for hiding this comment

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

However…

image

So, we can't do here too much.

@panr panr requested a review from pomek August 18, 2020 16:05
label: t( 'Insert image via URL' )
} );
labeledInputView.fieldView.placeholder = 'https://example.com/src/image.png';
labeledInputView.infoText = t( 'Paste the image source URL' );
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
labeledInputView.infoText = t( 'Paste the image source URL' );
labeledInputView.infoText = t( 'Paste the image source URL.' );

Don't forget about updating contexts.json.

Copy link
Member

Choose a reason for hiding this comment

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

Same in L156. Missing the dot at the end of the sentence.

Copy link
Contributor Author

@panr panr Aug 19, 2020

Choose a reason for hiding this comment

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

Same in L156. Missing the dot at the end of the sentence.

But it's a form label. It shouldn't end with the dot 🤔

@panr panr requested a review from pomek August 19, 2020 10:00
@panr
Copy link
Contributor Author

panr commented Aug 19, 2020

@pomek changes applied, I hope it will be ✅ 👀

Copy link
Member

@pomek pomek left a comment

Choose a reason for hiding this comment

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

LGTM.

@pomek

This comment has been minimized.

@Reinmar Reinmar self-requested a review August 20, 2020 07:40
@Reinmar
Copy link
Member

Reinmar commented Aug 20, 2020

I've got doubts regarding how this feature should be proposed – as a separate plugin, an opt-in feature of the current UploadImage plugin or an opt-out feature of it.

I'll write a separate RFC for that.

@Reinmar
Copy link
Member

Reinmar commented Aug 20, 2020

We made a decision regarding how to propose this feature: #7794 (comment).

@panr, could you adjust the code? It may require a bit more changes as this will be a button or a split button depending on the config option.

@panr
Copy link
Contributor Author

panr commented Aug 20, 2020

@Reinmar did the changes, now I'm on the docs update.

@Reinmar Reinmar merged commit bb00c23 into master Aug 20, 2020
@Reinmar Reinmar deleted the i/7794 branch August 20, 2020 21:54
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.

Implement inserting image via URL
3 participants