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

Document image style migration scenarios and default styles #9339

Closed
oleq opened this issue Mar 23, 2021 · 9 comments · Fixed by #9379
Closed

Document image style migration scenarios and default styles #9339

oleq opened this issue Mar 23, 2021 · 9 comments · Fixed by #9379
Assignees
Labels
package:image squad:core Issue to be handled by the Core team. type:docs This issue reports a task related to documentation (e.g. an idea for a guide).

Comments

@oleq
Copy link
Member

oleq commented Mar 23, 2021

📝 Provide a description of requested docs changes

@oleq oleq added type:docs This issue reports a task related to documentation (e.g. an idea for a guide). package:image labels Mar 23, 2021
@oleq oleq added this to the iteration 42 milestone Mar 23, 2021
@magda-chrzescian
Copy link
Contributor

Some guidelines agreed at the meeting:

  • Way to think about it – someone has a certain setup (preset + configuration) and upgrades to the new version where we changed the default settings. There must be a way to:
    • Ensure that the content previously created will be maintained.
    • Which, in the simplest scenario, boils down to configuring the new version of the editor in the same way it was configured previously.
    • However, an additional thing to consider is – what if someone wants the same setup they had, but also use the new inline images.
  • Scenarios to cover:
    • Article (Classic/Balloon/Inline) → new version (new preset)
      • Must be able to set the styles to side and full and remove inline images (assuming it's added to the build in the new version)
      • Bonus: Remember that side-image must apply max-width:50%
      • Bonus: How to remove InlineImage from a build? May require re-building.
    • Article → new version + inline images (new preset)
      • The same block styles (side and full) + something reasonable for inline images
    • Document → new version
      • Document has left, full, right currently
      • It must be able to set the styles to left, full, right and remove inline images
    • Document → new version + inline images
      • Document has left, full, right currently
      • Two options here:
        • Will the new default configuration work?
        • Or does it have to be adjusted?
    • Custom styles → new version + ?
      • Are there any tricky scenarios that were possible to achieve in the previous version of the feature but will not be easy to do now?

@magda-chrzescian magda-chrzescian added the squad:core Issue to be handled by the Core team. label Mar 29, 2021
@magda-chrzescian
Copy link
Contributor

It may be worth mentioning that we are slightly modifying the content styles by removing clear: both rule from the image class (#9183).

@magda-chrzescian magda-chrzescian linked a pull request Mar 30, 2021 that will close this issue
@magda-chrzescian
Copy link
Contributor

A few words about renaming the public functions.

We actually don't need the getSelectedImageWidget function anymore. Whenever we are looking for an image on which to execute a command, we are also looking for the image ancestor of the selected caption. So I replaced the getSelectedImageWidget with getSelectedOrAncestorImageWidget.

It was a public function, so I'm not sure if it's ok to change its name and behavior. But because of the changes in managing the toolbar state, our integrator probably also should start using the getSelectedOrAncestorImageWidget function.

BTW, should such renaming be considered a breaking change?

@magda-chrzescian
Copy link
Contributor

The declarative drop-downs should be mentioned here.

@magda-chrzescian
Copy link
Contributor

Let's remember about mentioning changes from #9675 here.

@oleq
Copy link
Member Author

oleq commented Jun 25, 2021

Although #9379 was merged, there are few things left to do in this issue:

@godai78
Copy link
Contributor

godai78 commented Jun 25, 2021

I'll be proofreading migration guide today.

@oleq
Copy link
Member Author

oleq commented Jun 25, 2021

Some things I came across while reading the migration guide:

  • The ImageInline is a newly introduced plugin supporting an inline <img> tag nested in any $block element in the editor. In the model, it is represented by an imageInline element.

I'm not sure 100% this is true BC imageInline has allowWhere: '$text', and there are $blocks such as media or pageBreak that don't allow $text. I think it's enough to say that 

[...] supporting an inline <img> tag nested in text (e.g. inside a paragraph).


The ImageBlock maintains the functionality of the previous Image plugin. In the model, the previous image element is renamed to imageBlock.

For clarity, I'd rewrite it to:

The ImageBlock maintains the functionality of the Image plugin before v29.0.0. In the model, it uses the imageBlock element (known as image before v29.0.0).


To provide a valid HTML data output, an image caption is supported for the block images only. Adding a caption to an inline image results in conversion the block image.

For clarity, I'd rewrite this to:

To provide a valid HTML data output, an image caption is supported for the block images only. Adding a caption to an inline image will automatically convert it to the block image (which can be undone by the user).


the default content styles for these kind of images remains the same

to

the default content styles for these images remain the same


The image toolbar section starting with

Due to the changes mentioned above, the image toolbar became crucial in terms of providing the user with proper interaction with images in terms of managing the image type and caption. Thus, it is recommended to use one of the following configurations as the minimum set-up for the image toolbar:

is unclear to me. Do I need to configure this in v29.0.0+ or the feature will not work? It's more an announcement than migration to me. I think we need to think this over.


For clarity, I'd rewrite

When loaded, the ImageInline plugin changes the default behavior of inserting/pasting/dropping images into a non-empty $block elements - it is now upcasted to the imageInline model element. The image inserted into an empty paragraph is still upcasted to a imageBlock model element. This behavior can be overridden in the ImageInsert plugin configuration to force an insertion block or inline images only.

to

Since v29.0.0 inserting (also: pasting, dropping) an image in the middle of text will no longer split it if the ImageInline plugin is loaded (default). If you prefer the old behavior in your integration, this can be specified in the ImageInsert plugin configuration.


Read more about the [logic controlling the image type while inserting/pasting/dropping](TODO: link).

😛


The image utils are now wrapped by a ImageUtils plugin.

to

The image utils are now wrapped by the ImageUtils plugin.


// Old code
// New code

to

// Before v29.0.0
// Since v29.0.0


to


  • The isImage function returns now an Element for both, inline and block images.

is not true. This should be:

  • The isImage function works for both inline and block images.

to


In general, we end the names of methods/helpers with () so instead of getSelectedImageWidget, we should go with getSelectedImageWidget() etc.. This should be fixed across the guide.

@oleq
Copy link
Member Author

oleq commented Jun 29, 2021

Closed in #9952.

@oleq oleq closed this as completed Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:image squad:core Issue to be handled by the Core team. type:docs This issue reports a task related to documentation (e.g. an idea for a guide).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants