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

New UI for insert image feature #15255

Merged
merged 56 commits into from
Dec 5, 2023
Merged

Conversation

niegowski
Copy link
Contributor

@niegowski niegowski commented Oct 26, 2023

Suggested merge commit message (convention)

Feature (image): Introduced the image insert dropdown as a consistent UI to insert images through different available integrations like image upload, insert image with asset manager, and insert image via URL. Closes #15303. Closes #15149.

Other (image): The ImageUploadUI plugin is loaded by default when the ImageBlock or ImageInline plugins are loaded. See #15149.

Other (list, ui): The CollapsibleView moved from the list package to the ui package. See #15149.

Other (ui): The SplitButtonView constructor and createDropdown() helper accepts an instance of a ButtonView as an action view customization. See #15149.

Other (upload): The FileDialogButtonView is now an instance of the ButtonView, not just a wrapper on it. See #15149.

Internal (ckbox, ckfinder, image): The ImageUploadUI, CKBoxUI, CKFinderUI, and ImageInsertViaUrlUI plugins are registering integrations in the ImageInsertUI.

Internal (core): Added icons for ImageUploadUI and its integrations. Added translation contexts for common labels for images insert/replace via file manager.

MINOR BREAKING CHANGE (list): The CollapsibleView moved from the list package to the ui package. You can import it like this: import { CollapsibleView } from '@ckeditor/ckeditor5-ui';


Additional information

@niegowski niegowski changed the title PoC of declarative nested toolbar dropdown with split button. New UI for insert image feature Nov 13, 2023
@niegowski niegowski marked this pull request as ready for review December 4, 2023 19:34
@niegowski niegowski requested a review from pszczesniak December 5, 2023 09:05
Copy link
Contributor

@pszczesniak pszczesniak left a comment

Choose a reason for hiding this comment

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

LGTM 👍

One missing description - but it's not a blocker.

@@ -32,7 +32,7 @@
}

& > .ck-collapsible__children {
padding: 0 var(--ck-spacing-large) var(--ck-spacing-large);
padding: var(--ck-spacing-medium) var(--ck-spacing-large) var(--ck-spacing-large);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why top padding is changed from large into medium? When button is focused it looks odd when label in on the box-shadow outline:
 
Screenshot 2023-12-05 at 15 44 30

But as i understand decision was made - i just wanted to show my doubts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's increased from 0 to medium 😀

Copy link
Contributor

Choose a reason for hiding this comment

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

Missed the 0 , sorry! but still, large looks a bit better IMO ;)

Screenshot 2023-12-05 at 16 17 17

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When it's larger it looks strange when the button outline is not visible.

@pszczesniak
Copy link
Contributor

One side comment from me:

I was/am a bit confused when two icons/buttons with same functionality are labeled differently:

Don't know why it is done this way.

@niegowski
Copy link
Contributor Author

Don't know why it is done this way.

To give context. In the dropdown, the user already has a context that it's a dropdown to insert an image. The main button does not have the context, so an extended label is used to make it clear that it's inserting images, not any file.

Copy link
Contributor

@mmotyczynska mmotyczynska left a comment

Choose a reason for hiding this comment

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

Looks and works good 👍

Only questions regarding types (in comments).

@@ -57,5 +59,43 @@ export default class CKBoxUI extends Plugin {

return button;
} );

if ( editor.plugins.has( 'ImageInsertUI' ) ) {
const imageInsertUI: ImageInsertUI = editor.plugins.get( 'ImageInsertUI' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this explicit type ImageInsertUI needed in this case?

Comment on lines +59 to +60
const imageInsertUI: ImageInsertUI = editor.plugins.get( 'ImageInsertUI' );
const command: CKFinderCommand = editor.commands.get( 'ckfinder' )!;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these explicit types needed here?

return this._createDropdownView( locale );
};
const selection = editor.model.document.selection;
const imageUtils: ImageUtils = editor.plugins.get( 'ImageUtils' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this explicit type needed here?

@niegowski niegowski merged commit 0647ba6 into master Dec 5, 2023
@niegowski niegowski deleted the ck/15195-toolbar-nesting-split branch December 5, 2023 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants