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

Implement inserting image via URL #7794

Closed
panr opened this issue Aug 6, 2020 · 6 comments · Fixed by #7835
Closed

Implement inserting image via URL #7794

panr opened this issue Aug 6, 2020 · 6 comments · Fixed by #7835
Assignees
Labels
domain:ui/ux This issue reports a problem related to UI or UX. package:image squad:core Issue to be handled by the Core team. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@panr
Copy link
Contributor

panr commented Aug 6, 2020

📝 Provide a description of the new feature

The summary begins here: #7439 (comment).

@panr panr added type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:image domain:ui/ux This issue reports a problem related to UI or UX. labels Aug 6, 2020
@panr panr added this to the iteration 35 milestone Aug 6, 2020
@panr panr self-assigned this Aug 6, 2020
@panr panr added the squad:core Issue to be handled by the Core team. label Aug 7, 2020
@Reinmar
Copy link
Member

Reinmar commented Aug 20, 2020

RFC: Should the insert image via XYZ be an opt-in/opt-out feature of the UploadImage plugin or a separate plugin?

Context

So far, we've got the ImageUpload plugin that registers the uploadImage button that when clicked opens the native file browser.

In this iteration, we're adding a split button that:

  • when its face is clicked does what the original uploadImage does,
  • when the arrow is clicked shows the "insert via URL" and other integratrions (CKFinder) in a panel.

How should this new feature be proposed?

Considered options

  • Separate plugin
    • ImageInsert or something.
    • It could override the base uploadImage button the same way that ListStyle overrides List.
    • Or it could register a new component as uploadImage was about upload while the new dropdown is about more kinds of insertions.
  • Opt-in feature of the current ImageUpload plugin.
    • It could be driven by whether the config.image.panel is set. It'd default to null to make this opt-in.
  • Opt-out feature of the current ImageUpload plugin.

Pros and cons

  • Separate plugin
    • Pros:
      • Granular – you load only what you need.
      • Opt-in by default, so we don't affect any existing setups.
      • Consistent with ListStyle if we'd go with overriding the current uploadImage button, but I'd rather go with a new button insertImage anyway as uploadImage was too specific.
    • Cons:
      • Overriding one UI component with another is not the greatest pattern of all time.
      • More complexity as the code will be scattered over more places.
      • Refactoring needs time, so this plugin won't make it to the release.
  • Opt-in feature of the current ImageUpload plugin
    • Pros:
      • We don't affect any existing setup.
      • No complexity other than a single if().
    • Cons:
      • No granularity.
      • The ImageUpload is about... upload. Suddenly, with this extension, it will be about everything (all kinds of sources).
  • Opt-out feature of the current ImageUpload plugin
    • Just like opt-in, but we affect the existing setups.

Proposal

I'm hesitating between a separate plugin and opt-in feature of ImageUpload. If we had time, I'd definitely go with a separate plugin as it's more future-safe and does not deteriorate the current naming. But it got very late so I started considering the opt-in version. It comes with significant tradeoffs, though.

@jodator
Copy link
Contributor

jodator commented Aug 20, 2020

I'm for:

  1. Separate plugin in the long term (next iteration).
  2. Whatever opt-in is the fastest to implement.
    1. going with opt-in might be as "secret" / feature flag for this release with a clear intention (stated in the docs) that this is a temporal solution, Also this feature might be advertised as feature preview (justification for a temporal config to enable).
      This will not block the release.
      The feature configuration will be prepared for a Separate plugin approach.
  3. Splitting the repo to multirepo, add this as a separate plugin, release later... :trollface:. (ps.: I'm not for this, monorepo FTW!)

@Reinmar
Copy link
Member

Reinmar commented Aug 20, 2020

Whatever opt-in is the fastest to implement.

  1. going with opt-in might be as "secret" / feature flag for this release with a clear intention (stated in the docs) that this is a temporal solution, Also this feature might be advertised as feature preview (justification for a temporal config to enable).
    This will not block the release.
    The feature configuration will be prepared for a Separate plugin approach.

That sounds good.

So, a new proposal:

config.image.upload.panel should be false by default.

Setting it to true (or any truthy value because we actually already support the { items: [] } thing) should turn the normal button into a split button.

We'll explain it in the docs that it's temporal.

@oleq
Copy link
Member

oleq commented Aug 20, 2020

config.image.upload.panel

config.image.upload.showPanel or config.image.upload.displayOptionsPanel or something more verbose than plain panel?

@Reinmar
Copy link
Member

Reinmar commented Aug 20, 2020

config.image.upload.showPanel or config.image.upload.displayOptionsPanel or something more verbose than plain panel?

That'd be ideal if this option didn't actually exist already and supported that panel's configuration. OTOH, it's actually protected anyway so... maybe you're right and we can add another option just as a flag to show or hide this feature. In other words – there'd be two options for the time being. One public (`displayOptionsPanel`) and the other protected.

@panr
Copy link
Contributor Author

panr commented Aug 20, 2020

What about avoiding ...options madness? ;-P

Reinmar added a commit that referenced this issue Aug 20, 2020
Feature (image): Introduced the insert image via URL feature. Closes #7794.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:ui/ux This issue reports a problem related to UI or UX. package:image squad:core Issue to be handled by the Core team. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants