t/134: Various improvements to config.image.styles #140

oleq · 2017-09-13T13:40:43Z

Suggested merge commit message (convention)

Feature: Allowed customization of default image styles. Defined formatting–oriented styles. Simplified config.image.styles syntax. Closes ckeditor/ckeditor5#5113. Closes ckeditor/ckeditor5#5114.

BREAKING CHANGE: From now on, the imageStyleFull uses object-full-width.svg icon.

Additional information

Requires:

https://github.com/ckeditor/ckeditor5-core/tree/t/ckeditor5-image/134 (additional icon).
https://github.com/ckeditor/ckeditor5/tree/t/ckeditor5-image/134 (docs and CI)

Showcase in http://localhost:8125/ckeditor5-image/tests/manual/imagestyle.html.

Default config:

image: {
	styles: [ 'imageStyleFull', 'imageStyleSide' ]
}

Customizing just the icon and class in defaults:

image: {
	styles: [ 
		{ name: 'imageStyleFull', icon: 'left' }, 
		{ name: 'imageStyleSide', className: 'foo' } 
	]
}

Completely custom styles also benefit from translations and default icons:

image: {
	styles: [
		// This style's title will be translated, e.g. to german. It uses default right icon.
		{ name: 'foo', title: 'Full size image', icon: 'right', value: null, className: 'foo-class' },
		// This style does everything in a custom way.
		{ name: 'bar', title: 'bar', icon: '<svg>...</svg>', value: 'bar', className: 'bar-class' },
	]
}

….image.styles with translations, ImageStyleEngine.defaultStyles extension and ImageStyleEngine.defaultIcons support.

…StylesTitles.

…tyles.

… icons in different cases.

…t-full-width icon.

wwalc · 2017-09-14T07:23:51Z

Any chance the default CSS rules for imageStyleAlignLeft could play better by default with the content inside CKEditor? I used the default browser styles and rendered Classic Editor inside. I used blockquote and bulleted list and both look bad:

Align right (ok)

Align left (could be better)

Not sure if it's an issue with ckeditor5-image or related plugins responsible for blockquote/lists.
However anyone using these styles will face that problem so it would be nice to come with some solution.

oleq · 2017-09-14T08:04:26Z

For one thing, I could increase the default margin around the images.

Then there's the problem with lists and blockquotes that we faced recently. It boils down to overflow: hidden for <ol> and <blockquote> to enable block formatting. CKE4 is suffering because of that too (and the quote most of all):

Reinmar · 2017-09-14T08:08:19Z

I'm not sure how far we can go. We can increase margins – that's fine. But can we use overflow:hidden? That has, as we learnt, a pretty unclear effect. Developers may be confused by this.

OTOH, I'd love if the initial styling was perfect...

oleq · 2017-09-14T08:18:22Z

OTOH, I'd love if the initial styling was perfect...

I'm afraid there's nothing we can do. It's overflow, or nothing :/

There's a long discussion on this topic https://stackoverflow.com/questions/710158/why-do-my-list-item-bullets-overlap-floating-elements.

Reinmar · 2017-09-14T08:19:40Z

Hm... OK, seeing this I'm fine with overflow :D

fredck · 2017-09-14T13:31:31Z

You mean fine, as long as the text of long paragraphs wrap around the image, right?

wwalc · 2017-09-15T08:46:42Z

TL;DR:

I'd like to propose the following:

Increase the right margin for .ck-editor__editable .image.image-style-align-left
Apply overflow: hidden for blockquotes.

Reasoning below.

I guess there is no perfect solution. Thinking out loud: in terms of frequency of using semantic block structures I guess the order will be like:

paragraphs
headings
lists
... long break ...
quotes

Paragraphs, headings

There is no issue to solve.

Lists

Bigger margin

.ck-editor__editable .image.image-style-align-left {margin-right: 2em}

Pros: the bullets are visible.
Pros: plays well with long lists.
Pros: the heading has better margin, IMO.

Overflow: hidden

Pros: the bullets are visible.
Cons: I don't like the significant difference between heading and lists margin.
Cons: in case of very long lists looks weird. Personally I create long lists quite often.

IMO: bigger margin wins

Quotes

No changes

Bigger margin (to solve issue with lists)

Overflow: hidden + bigger margin (to solve issue with lists)

Pros: in this case at least shorter quotes look okay. If someone wants to use longer quotes he will have to use either centered/full size image or align it to right, there is no better solution.

IMO: Overflow: hidden + bigger margin wins.

Reinmar · 2017-09-15T08:56:23Z

Agree. I didn't think that overflow:hidden will make long list indented on the entire height of them. This is bad for lists, but makes sense for quotes.

wwalc · 2017-09-15T11:53:19Z

It looks like defining a completely custom style does not work, the className is not respected.

ClassicEditor
  .create( document.querySelector( '#editor' ), {
    image: {
      toolbar: [ 'imageTextAlternative', '|', 'imageStyleCustom', 'imageStyleAlignCenter', 'imageStyleAlignRight' ],

      styles: [
         // THIS ONE DOES NOT WORK
         { name: 'imageStyleCustom', icon: 'left', className: 'foo-class', title: 'Custom title' },
         'imageStyleAlignCenter',
         { name: 'imageStyleAlignRight', className: 'bar-class' }
      ]
    }
  })
.catch( error => {
  console.error( error );
} );

imageStyleCustom correctly shows up in the image toolbar, but the class is not applied. Note that overriding className for imageStyleAlignRight works.

oleq · 2017-09-15T13:45:11Z

@wwalc Does it show up in editor.plugins.get( ImageStyleEngine ).imageStyles (the class)?

wwalc · 2017-09-15T16:08:03Z

@oleq I modified the manual test in http://localhost:8125/ckeditor5-image/tests/manual/imagestyle.html

ClassicEditor
	.create( document.querySelector( '#editor-semantic' ), {
		plugins: [
			ImageToolbar,
			EnterPlugin,
			TypingPlugin,
			ParagraphPlugin,
			HeadingPlugin,
			ImagePlugin,
			UndoPlugin,
			ClipboardPlugin,
			ImageStyle
		],
		toolbar: [ 'headings', 'undo', 'redo' ],
		image: {
			// You need to configure the image toolbar too, so it uses the new style buttons.
			toolbar: [ 'imageTextAlternative', '|', 'imageStyleCustom', 'imageStyleAlignCenter', 'imageStyleAlignRight' ],

			styles: [
				{ name: 'imageStyleCustom', icon: 'left', className: 'foo-class', title: 'Custom title' },
				'imageStyleAlignCenter',
				{ name: 'imageStyleAlignRight', className: 'bar-class' }
			]
		}
	} )
	.then( editor => {
		window.editorSemantic = editor;
	} )
	.catch( err => {
		console.error( err.stack );
	} );

With such test, in Firefox in the console I have the following results:

Note: I initially reproduced the same issue in a manually created classic build. So I can confirm it using both a build and a modified manual test.

oleq · 2017-09-19T11:25:05Z

@wwalc

{ name: 'imageStyleCustom', icon: 'left', className: 'foo-class', title: 'Custom title' },

does not work because it has no value defined. I'm wondering if we should log.warn in such case 🤔 Or at least make it clear in the documentation.

…better interaction with default list styles.

oleq · 2017-09-19T11:43:44Z

I increased the default image margins for better interaction with lists. An overflow for blockquote should be added in a followup in the ckeditor5-block-quote repository.

BTW, I found a funny bug in Chrome. Can you also reproduce it? I hope it's not CKEditor–specific.

wwalc · 2017-09-19T12:24:23Z

does not work because it has no value defined. I'm wondering if we should log.warn in such case 🤔 Or at least make it clear in the documentation.

Doh. It's fine. I have no idea why I missed the value property. I must have copied something that did not contain value defined at all.

BTW. Wouldn't it be more intuitive is the default value was set to whatever the className is set? Or to something like class-className? After all when defining styles I wouldn't care most of the time about some internal model attribute. Also isn't there a chance that when asking users to define manually such thing they may break something else (e.g. using title, alt or whatever as a value)?

oleq · 2017-09-19T13:19:20Z

TBH, I also find it a little bit confusing that the value must always be provided as a null|String.

Do you remember why we decided to go this way, @szymonkups? Can we do anything about this?

szymonkups · 2017-09-19T13:41:13Z

Do you remember why we decided to go this way, @szymonkups? Can we do anything about this?

This is implemented this way just to provide a way to configure model value for the style. Looking at this now I think @wwalc opinion has more sense to treat it as some internals that should be hidden from the configurator. Value is stored inside known attribute so I think that it is safe to create value based on style name and/or class name. This will require to mark default style in some different way - currently by setting null as the value.

Reinmar · 2017-09-19T15:27:08Z

This will require to mark default style in some different way - currently by setting null as the value.

Yup. Good point.

Agree with the rest too.

… #name instead. Defined ImageStyleFormat#isDefault.

wwalc · 2017-09-20T10:12:55Z

src/imagestyle/converters.js

@@ -48,7 +48,7 @@ export function modelToViewStyleAttribute( styles ) {
 */
 export function viewToModelStyleAttribute( styles ) {
 	// Convert only styles without `null` value.


Is the comment still valid here?

oleq · 2017-09-20T10:16:43Z

I got rid of ImageStyleFormat#value in ad1a540. Please let me know if it works for you and does not break anything.

It also means some fixes in documentation are needed in the ckeditor5 repo https://github.com/ckeditor/ckeditor5/tree/t/ckeditor5-image/134.

I also enabled CI in https://github.com/ckeditor/ckeditor5/tree/t/ckeditor5-image/134.

wwalc

I did not check the whole PR deeply, but the things that I requested to be corrected seem to work okay now. I'll review and update all the image docs once this PR merged.

oleq · 2017-09-20T13:45:01Z

@szymonkups Could you review this issue?

Reinmar · 2017-09-20T20:03:58Z

I'd start from the fact that CI for this branch fails :P

Reinmar · 2017-09-20T20:04:57Z

Oopsie – I haven't noticed that this feature requires changes in other pkgs.

oleq added 9 commits September 13, 2017 13:07

Defined ImageStyleEngine#imageStyles() getter which normalises config…

a94d681

….image.styles with translations, ImageStyleEngine.defaultStyles extension and ImageStyleEngine.defaultIcons support.

Made ImageStyleEngine.localizedDefaultStylesTitles a public getter.

e0bdb76

Docs: Extended documentation of the ImageStyleEngine.localizedDefault…

c860972

…StylesTitles.

Docs: Updated ImageConfig#styles with the new config syntax.

f83e2e0

Code refactoring: Renamed certain styles in ImageStyleEngine.defaultS…

4991d22

…tyles.

Docs: Simplified the 'Configuring Image Styles' feature overview.

87e9aa0

Added styles for left-aligned, right-aligned and centered image.

f654cfa

Tests: Updated the image style manual test with formatting styles.

9df032f

Made the ImageStyleEngine use the object-center and object-full-width…

8443396

… icons in different cases.

oleq requested review from Reinmar, wwalc and szymonkups September 13, 2017 13:40

Tests: Updated ImageStyleEngine test to assert the usage of the objec…

3dfa4b5

…t-full-width icon.

Tests: Fixed unnecessary changes to the image caption manual test.

47e24ab

oleq added 2 commits September 19, 2017 13:09

Merge branch 'master' into t/134

16c6f72

Added static pluginName getter to ImageStyleEngine. Minor docs fixes.

1f02a5e

Increased default margin for side, left and right-aligned images for …

1285710

…better interaction with default list styles.

Removed #value from ImageStyleFormat to simplify configuration. Using…

ad1a540

… #name instead. Defined ImageStyleFormat#isDefault.

wwalc reviewed Sep 20, 2017

View reviewed changes

Docs: Fixed outdated comment in the viewToModelStyleAttribute helper.

c8a2edc

wwalc approved these changes Sep 20, 2017

View reviewed changes

szymonkups added 2 commits September 21, 2017 16:27

Minor docs fix.

463a818

Merge branch 'master' into t/134

1672f28

szymonkups merged commit eab98ef into master Sep 21, 2017

szymonkups deleted the t/134 branch September 21, 2017 14:35

Reinmar mentioned this pull request Oct 2, 2017

[Edge] The list displays incorrectly near the left aligned image ckeditor/ckeditor5#576

Closed

oleq mentioned this pull request Jan 16, 2019

Table selection handler should not be truncated inside a block quote. ckeditor/ckeditor5-block-quote#30

Closed

oleq mentioned this pull request Oct 9, 2019

Improve default styles to integrate block quotes with floated objects ckeditor/ckeditor5#3319

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

t/134: Various improvements to config.image.styles #140

t/134: Various improvements to config.image.styles #140

oleq commented Sep 13, 2017 •

edited by pomek

Loading

wwalc commented Sep 14, 2017

oleq commented Sep 14, 2017

Reinmar commented Sep 14, 2017

oleq commented Sep 14, 2017 •

edited

Loading

Reinmar commented Sep 14, 2017

fredck commented Sep 14, 2017

wwalc commented Sep 15, 2017

Reinmar commented Sep 15, 2017

wwalc commented Sep 15, 2017 •

edited

Loading

oleq commented Sep 15, 2017 •

edited

Loading

wwalc commented Sep 15, 2017 •

edited

Loading

oleq commented Sep 19, 2017 •

edited

Loading

oleq commented Sep 19, 2017

wwalc commented Sep 19, 2017 •

edited

Loading

oleq commented Sep 19, 2017

szymonkups commented Sep 19, 2017 •

edited

Loading

Reinmar commented Sep 19, 2017

wwalc Sep 20, 2017

oleq commented Sep 20, 2017 •

edited

Loading

wwalc left a comment

oleq commented Sep 20, 2017

Reinmar commented Sep 20, 2017

Reinmar commented Sep 20, 2017

t/134: Various improvements to config.image.styles #140

t/134: Various improvements to config.image.styles #140

Conversation

oleq commented Sep 13, 2017 • edited by pomek Loading

Suggested merge commit message (convention)

Additional information

wwalc commented Sep 14, 2017

Align right (ok)

Align left (could be better)

oleq commented Sep 14, 2017

Reinmar commented Sep 14, 2017

oleq commented Sep 14, 2017 • edited Loading

Reinmar commented Sep 14, 2017

fredck commented Sep 14, 2017

wwalc commented Sep 15, 2017

Paragraphs, headings

Lists

Bigger margin

Overflow: hidden

Quotes

No changes

Bigger margin (to solve issue with lists)

Overflow: hidden + bigger margin (to solve issue with lists)

Reinmar commented Sep 15, 2017

wwalc commented Sep 15, 2017 • edited Loading

oleq commented Sep 15, 2017 • edited Loading

wwalc commented Sep 15, 2017 • edited Loading

oleq commented Sep 19, 2017 • edited Loading

oleq commented Sep 19, 2017

wwalc commented Sep 19, 2017 • edited Loading

oleq commented Sep 19, 2017

szymonkups commented Sep 19, 2017 • edited Loading

Reinmar commented Sep 19, 2017

wwalc Sep 20, 2017

Choose a reason for hiding this comment

oleq commented Sep 20, 2017 • edited Loading

wwalc left a comment

Choose a reason for hiding this comment

oleq commented Sep 20, 2017

Reinmar commented Sep 20, 2017

Reinmar commented Sep 20, 2017

oleq commented Sep 13, 2017 •

edited by pomek

Loading

oleq commented Sep 14, 2017 •

edited

Loading

wwalc commented Sep 15, 2017 •

edited

Loading

oleq commented Sep 15, 2017 •

edited

Loading

wwalc commented Sep 15, 2017 •

edited

Loading

oleq commented Sep 19, 2017 •

edited

Loading

wwalc commented Sep 19, 2017 •

edited

Loading

szymonkups commented Sep 19, 2017 •

edited

Loading

oleq commented Sep 20, 2017 •

edited

Loading