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

Image feature might need clean-up #8002

Closed
jodator opened this issue Sep 2, 2020 · 2 comments
Closed

Image feature might need clean-up #8002

jodator opened this issue Sep 2, 2020 · 2 comments
Labels
bc:major Resolving this issue will introduce a major breaking change. package:image resolution:expired This issue was closed due to lack of feedback. status:stale type:refactor This issue requires or describes a code refactoring.

Comments

@jodator
Copy link
Contributor

jodator commented Sep 2, 2020

After looking at the toolbar and the code:

  • it's an image related button/dropdown
  • by default it uploads image (main button)
  • it allows also inserting an image via dropdown
  • it allows to insert an image via file manager
  • it allows to change currently selected image (by upload or URL or external thing)

So, for me, I think it might be beneficial to clear things up here. Similar cleanup (mostly for utils) was done in tables recently because we agreed that we don't have much to add there in the foreseeable future and adding new features there will not happen soon. It might be a similar case for an image, in terms that we might have a solid set of image features for the foreseeable future.

5 min proposal:

  • Image - base image plugin, should provide
    • image editing features
    • inserting image command (a base command that should handle cases of inserting a image by providing its URL/URI - data image case) and updating image source
  • ImageUI - base general UI
    • provides extensibility (wild idea image ui component, which is behaves differently depending on a set of enabled features)
    • additional insertImage UI - solely responsible for inserting image via URL
  • adding ImageUpload enables uploading images
    • adds ImageUploadUI - enables UI related parts (potential to have upload commands without UI)
      • additional uploadImage button - solely responsible for uploading
    • adds ImageUploadEditing - enables commands, temporal upload handling in the editing area
  • adding InsertImageUI enables inserting image via URL part of UI
  • on the side there's CKFinder feature which should provide own, dedicated UI (as insertImage and uploadImage does) so one could also have separate button for insertImageWithCKFinder (I don't recall its button name).

Open question still "how the two-step process of uploading an image should be done behind the scenes?".

  • Right now it is a shared util between insertImage and uploadImage.
  • I'd image that expected way is that ImageUploadCommand either extends the behavior of the ImageInsertCommand or uses ImageInsertCommand to insert an image into the editing area. (@panr - this might be a real answer to your question).

Pros:

  • cleared up state of things, no waking up in the middle of the night form a bad dreams
  • flexibility of the toolbar composition
    • either one do-it all image
    • set of atomic UI parts for each of concern to pick & choose: insertImage, uploadImage, ckfinderImage.

Cons:

  • added time to the feature completeness state
  • too short analysis - I've might miss something

Looking at this in a broader spectrum - I think that we started from imageUpload button and build on top of it. I'd go with clearing things up if we see that the set of features of image feature is complete enough or planned image features align to the above.

Originally posted by @jodator in #7890 (comment)


Why we stayed as is: #7890 (comment). (tl;dr - it is working OK and we have other priorities now).


Idea about API change for the feature configuration by @panr : #7890 (comment) (Integrations should be set as image.insert.integrations.

@jodator jodator added bc:major Resolving this issue will introduce a major breaking change. package:image type:refactor This issue requires or describes a code refactoring. labels Sep 2, 2020
@jodator jodator added this to the unknown milestone Sep 2, 2020
@pomek pomek removed this from the unknown milestone Feb 21, 2022
@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may still be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

We've closed your issue due to inactivity over the last year. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).

@CKEditorBot CKEditorBot added the resolution:expired This issue was closed due to lack of feedback. label Nov 12, 2023
@CKEditorBot CKEditorBot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bc:major Resolving this issue will introduce a major breaking change. package:image resolution:expired This issue was closed due to lack of feedback. status:stale type:refactor This issue requires or describes a code refactoring.
Projects
None yet
Development

No branches or pull requests

3 participants