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

Conceptual problems between image styles and image resize #7560

Closed
Reinmar opened this issue Jul 6, 2020 · 5 comments · Fixed by #7625
Closed

Conceptual problems between image styles and image resize #7560

Reinmar opened this issue Jul 6, 2020 · 5 comments · Fixed by #7625
Assignees
Labels
domain:dx This issue reports a developer experience problem or possible improvement. package:image type:docs This issue reports a task related to documentation (e.g. an idea for a guide). type:feature This issue reports a feature request (an idea for a new functionality or a missing option). type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@Reinmar
Copy link
Member

Reinmar commented Jul 6, 2020

Current situation

Currently, we feature 5 pre-defined image styles:

  • full size,
  • aside,
  • left aligned,
  • right aligned,
  • centered

All of them, except "full size" set max-width of an image to 50% via a stylesheet.

This conflicts with the optional image resize feature as you may now have:

  • A left aligned image, appearing at 50%, which size (according to the ImageResize command) is not 50% and hence the UI of ImageResize will show that this image has "Original size".
  • An image that's not full size and yet it says in the toolbar it is:

Background

The background does not make things simpler to explain. We're following the comfortable "semantical content" path as long as we can before entering the rocky "presentational" path to solve most burning issues for content authors.

Initially we offered only image styles as the clear and "correct" option to work with content. Once we added image resize, the conflict appeared as both features were not designed with each other in mind.

Additional problem comes from the fact that we want to promote the more semantical approach as long as possible. We could just easily have image alignment like any other editor, but it does not represent what we believe is good for the editing platform.

Unfortunately, as a result of iterative development over many many years, the current situation is too messy and we need to review it. These features don't work together, the documentation is misleading and you'd really need to understand existing options to figure out how to enable image alignment + image resizing in a correct way.

Proposed direction

I think that we could consider these two setups:

  • Semantical. With semantical image styles (e.g. the default full size image and aside image, but not limited to this) and constrained, class-based resize (represented in the UI e.g. by respective buttons),
  • Presentational. With image alignment (e.g. left, right, none/center) + inline style-based image resize (non-constrained, or constrained by the UI only).

The documentation should be organised accordingly. Instead of showing all options separately, show these two scenarios and how to setup the editor to either follow one or another path.

Semantical

The existing image styles feature is ready. The pre-defined image styles (full size, aside) are fine. There's no work needed here.

However, the constrained, class-based image resize is not available. We're working right now on buttons for resizing images (as an option/alternative to dragging) but we did not consider there the class-based approach. I think that we should consider it. Theoretically, we could promote the semantical way without it, just like we did so far – it'd only allow full size or aside images, but if we'd be able to extend that option with constrained resizing, I think it might become more appealing.

The tricky part here will be synchronising image styles with constrained resize. Should resize options be available when the "full size" option is chosen? If yes, should it be "unchecked" when the user chooses "50%"? Should clicking it again make the image 100% width? Should the transitions and combinations be configurable? Doesn't it boil down to having more image styles (full size, aside, 50%, 75%) and a map of allowed combinations of these (e.g. full size + 50% is not allowed, while aside + 75% is)?

Anyway, for now, this option will only offer image styles with two predefined options – full size and aside.

Presentational

The existing image styles feature can be used, however, the pre-defined left, right, center styles are wrong as they set max-width to 50%. The max-width must be removed from these styles. If anyone wants to use these styles without image resize, they should define the max-width themselves. This will, naturally be a breaking change, but I think that we can make it quite safely (no data loss here, just additional step to do when updating to the new version).

The image resize by dragging and buttons for resizing will then work as expected with those styles.

Action plan


If you'd like to see this improvement implemented, add a 👍 reaction to this post.

@Reinmar Reinmar added the type:improvement This issue reports a possible enhancement of an existing feature. label Jul 6, 2020
@Reinmar
Copy link
Member Author

Reinmar commented Jul 6, 2020

cc @panr @oleq @jodator @wwalc

@Reinmar Reinmar added domain:dx This issue reports a developer experience problem or possible improvement. package:image type:docs This issue reports a task related to documentation (e.g. an idea for a guide). type:feature This issue reports a feature request (an idea for a new functionality or a missing option). labels Jul 6, 2020
@Reinmar Reinmar added this to the nice-to-have milestone Jul 6, 2020
@jodator
Copy link
Contributor

jodator commented Jul 7, 2020

Side though on this - wouldn't a semantic vs presentational guide/icon in the docs helpful? The base idea is to showcase somehow which features (or which options) are more suited for semantic and which for a presentational editor. This can be a guide that describes differences and then lists features/options and/or an indicator in a feature guide that this feature is either semantic or presentational (or which configuration option is better suited for which).


General idea iswas to have a page (maybe features page is OK for that) which would act as a guide for features - a birds eye perspective to describe which features are "semantical" features (Highlight, Document Title?) and which are "presentional" (font-size).

But, after looking at all the features I couldn't find more examples of purely "semantical" featuers besides Highlight and ImageStyles configured in certain way.

So instead of a dedicated page that would describe which feature is "semantical" and which are not I would see that as a special kind of info-box that we could use in docs to mark such features.

In the end I'm not sure if we need that at all.

@Reinmar
Copy link
Member Author

Reinmar commented Jul 7, 2020

Oh, I like this idea. This could be informative, but it should not impose anything. We could link to such a guide from problematic places like the highlight feature, image styles, etc. The guide could help people understand some decisions that we made, how to live with semantical content, how to process it, how to implement their features, etc.

I'll open a separate ticket for that.

@Reinmar
Copy link
Member Author

Reinmar commented Jul 7, 2020

Done: #7571.

@jodator
Copy link
Contributor

jodator commented Jul 8, 2020

Notes from internal meeting:

  • keep separately settings for semantical and structure
  • remove max-width from image styles (make it in below 👇and describe for changelog).

@panr panr self-assigned this Jul 9, 2020
@jodator jodator modified the milestones: nice-to-have, iteration 34 Jul 13, 2020
Reinmar added a commit that referenced this issue Jul 21, 2020
Docs (image): Updated Image feature documentation for the ImageStyle and ImageResize plugins. Closes #7560.

Docs (image): Divided the ImageStyle documentation into semantical and presentational styles with corresponding snippets configuration.

Other (image): Image alignment styles (`alignLeft`, `alignCenter` and `alignRight`) no longer set `max-width: 50%` of the `<figure>` element.  If you wish them to still do so, add [these styles](https://github.com/ckeditor/ckeditor5/pull/7625/files#diff-960e3b5e24794dab54cce5dd955c2db2L11-L16) to your content styles.

BREAKING CHANGE (image): Image alignment styles (`alignLeft`, `alignCenter` and `alignRight`) no longer set `max-width: 50%` of the `<figure>` element. If you wish them to still do so, add [these styles](https://github.com/ckeditor/ckeditor5/pull/7625/files#diff-960e3b5e24794dab54cce5dd955c2db2L11-L16) to your content styles.

Tests (image): Updated manual tests configurations for the ImageResize feature.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:dx This issue reports a developer experience problem or possible improvement. package:image type:docs This issue reports a task related to documentation (e.g. an idea for a guide). type:feature This issue reports a feature request (an idea for a new functionality or a missing option). type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
3 participants