Add prototype submenu for image insert. #16334
Conversation
WalkthroughThe recent updates across multiple CKEditor5 packages focus on enhancing UI components, particularly through the addition of Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (8)
Files skipped from review due to trivial changes (2)
Additional comments not posted (6)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
Quick functional review (images below):
Ad 3:
Ad 4:
Ad 8:
|
Yep, good catch! I want this form gone :)
Which ui component is it? Shouldn't this be named
I agree, I think in main level in the menu bar we should keep nouns, and ideally don't change it to not confuse users. |
|
This is file manager component, but it wouldn't follow a convention if we name it "File manager" in the menu. The menu is called "Insert" and each label in the menu is an entity that you insert, e.g. "Insert -> table", "Insert -> image", etc. "Insert -> file manager" does not follow that. Hence I proposed "File". It could be "Insert -> From/Using/With file manager" 🤷 But I though "File" will be just cleaner. |
|
So maybe it doesn't make sense to keep it in Insert? File manager is also used for other things (CKBox), managing files, editing, upload is just one of the capabilities. |
|
I think you should have a menu bar button that allows you to insert a file and is easily discoverable. Not hidden under "Insert -> Image -> Image with file manager". We could also add "File manager" in "Tools -> File manager". But that would be a 3rd place where the same component is added 😄. Also, I don't know if we can even open the file manager in a different mode than "insert something". I mean, you always have "Insert" button in the dialog. You don't open editor, and file manager from the editor, to organize your files, right? (Unless I miss something.) |
|
Then let's go with the file. |
There was a problem hiding this comment.
Additionally go through all the files in the image package related to UI / inserting images and check if all docs are up-to-date.
E.g. ImageInsertUI has this description:
Adds the `'insertImage'` dropdown to the {@link module:ui/componentfactory~ComponentFactory UI component factory}
and also the `imageInsert` dropdown as an alias for backward compatibility.
Which doesn't even mention anything about menu bar.
packages/ckeditor5-image/src/imageinsert/imageinsertviaurlui.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-image/src/imageinsert/imageinsertviaurlui.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-image/src/imageinsert/imageinsertviaurlui.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-image/src/imageinsert/imageinsertviaurlui.ts
Outdated
Show resolved
Hide resolved
| buttonViewCreator: isOnlyOne => this._createInsertUrlButton( isOnlyOne ), | ||
| formViewCreator: isOnlyOne => this._createInsertUrlView( isOnlyOne ) | ||
| buttonViewCreator: () => this._createInsertUrlButton( ButtonView ), | ||
| formViewCreator: () => this._createInsertUrlView(), |
There was a problem hiding this comment.
This should look and behave similar to this._createInsertUrlButton( MenuBarMenuListItemButtonView ). It should be a dropdown item (button) like other integration methods.
| @@ -36,6 +42,8 @@ import ImageUtils from '../imageutils.js'; | |||
| * | |||
| * Adds the `'insertImage'` dropdown to the {@link module:ui/componentfactory~ComponentFactory UI component factory} | |||
| * and also the `imageInsert` dropdown as an alias for backward compatibility. | |||
| * | |||
| * Adds the `'menuBar:insertImage'` sub-menu to the {@link module:ui/componentfactory~ComponentFactory UI component factory}. | |||
There was a problem hiding this comment.
We also added new components in ckbox and ckfinder and AFAICS their API docs are not updated as well.
I also wanted you to go through existing guides but since core team is now reworking them AND we need to do other tasks related to docs (update demos, update all other guides not only images), then let's skip it for now.
|
Please remove tooltips from the items in the toolbar dropdown as they are simply repeating the button label and are annoying. Regarding labels and tooltips, as it seems that it got confusing:
Make sure that only toolbar buttons (and all of them!) have tooltips. |
| buttonViewCreator: isOnlyOne => this._createInsertUrlButton( isOnlyOne ), | ||
| formViewCreator: isOnlyOne => this._createInsertUrlView( isOnlyOne ) | ||
| requiresForm: false, | ||
| buttonViewCreator, |
There was a problem hiding this comment.
This is probably incorrect 🤔. If insert via URL is the only available option, then it will be displayed in the toolbar with text, right? Which is wrong.
There was a problem hiding this comment.
AFAIU, buttonViewCreator should use the uploadUrl component.
There was a problem hiding this comment.
Also, I just realized that uploadUrl is rather a bad name. First question: upload what? Second: we don't upload anything really.
The other components we have are called:
insertImageuploadImage
urlImage sounds quite bad so I propose insertImageUrl (and ofc menuBar:insertImageUrl).
There was a problem hiding this comment.
Also please add info about this component to metadata file. After the selected part:
Suggested merge commit message (convention)
Feature (image): Add menu bar integration for inserting an image via URL.
Minor Breaking Change (image): Inserting an image via URL is done in a modal instead of being directly in the image upload dropdown.
Minor Breaking Change (image): The image upload dropdown no longer supports focus cycling between child controls.
Additional information
For example – encountered issues, assumptions you had to make, other affected tickets, etc.