New image widget

[Olek Nowodziński]

Widgets feature is to be released with a powerful and spectacular image widget.

[Matti Järvinen]

Couple of mock screens or test URL would be awesome.

[Frederico Caldeira Knabben]

Replying to matti:

git clone https://github.com/cksource/ckeditor-dev.git
cd ckeditor-dev
git checkout t/10659

Then just open the sample in plugins/image2 ;)

[Frederico Caldeira Knabben]

Review for all changes between a373ebc and 7e9c628

• plugins/image2/dev/assets/audi.jpg
• plugins/image2/dev/assets/maserati.jpg
  • We need licenses for these images, or new ones.

• plugins/image2/dev/contents.css
  • Missing copyright notice.

• plugins/image2/dialogs/image2.js
  • “Lock ratio” should be on by default. It goes off if the image has sizes.

• plugins/image2/icons/hidpi/image2.png
• plugins/image2/icons/image2.png
  • I don’t think we need a new icon here. Image2 is a straight replacement for the Image plugin. It does the same job, better. We should use the same icon.

• plugins/image2/plugin.js
  • The caption should not allow <p> and I think that not even <br>. So, ENTER should not work on it.
  • Actually, we should have a very reduced ACF list here, supporting only inline elements.

• plugins/image2/samples/assets/audi.jpg
• plugins/image2/samples/assets/maserati.jpg
  • We need licenses for these images, or new ones.

• plugins/image2/samples/contents.css
  • Missing copyright notice.
  • The “h2” entry there is not required and can just cause confusion.

• plugins/image2/samples/image2.html
  • The sample page is not a test page. It must simply illustrate the feature:
  • There is no need to have a framed and a inline editor. Only one (framed) is enough.
  • Complicated internal stuff, like “upcasting”, should not be used in the sample text. E.g. “I'm centered while upcasted!”.

[Matti Järvinen]

Thanks looks awesome.

Replying to fredck:

• plugins/image2/icons/hidpi/image2.png
• plugins/image2/icons/image2.png
  • I don’t think we need a new icon here. Image2 is a straight replacement for the Image plugin. It does the same job, better. We should use the same icon.

With image2 plugin non-captioned image margins are handled with styles selection or is image2 plugin just widget feature demo?

Cannot make a link from an image. I know bad idea, but you know how customers are :)

If you can bake all this in Image2 it would be awesome, but until you do I'm not so sure about replacing the old one yet.

[Olek Nowodziński]

Replying to fredck:

Review for all changes between a373ebc and 7e9c628

• plugins/image2/dev/assets/audi.jpg
• plugins/image2/dev/assets/maserati.jpg
  • We need licenses for these images, or new ones.

Done: Used public domain images instead.

• plugins/image2/dev/contents.css
  • Missing copyright notice.

Done.

• plugins/image2/dialogs/image2.js
  • “Lock ratio” should be on by default. It goes off if the image has sizes.

It mimics old image's behavior. To be fixed soon.

• plugins/image2/icons/hidpi/image2.png
• plugins/image2/icons/image2.png
  • I don’t think we need a new icon here. Image2 is a straight replacement for the Image plugin. It does the same job, better. We should use the same icon.

Done: Reverted old ones.

• plugins/image2/plugin.js
  • The caption should not allow <p> and I think that not even <br>. So, ENTER should not work on it.
  • Actually, we should have a very reduced ACF list here, supporting only inline elements.

Done: Specified ACF rules. At the moment there's no way to prevent line breaks (i.e. ENTER_NONE) so I also included <br>. Note that ACF for widget editables is still very buggy: despite of rules specified, Advanced Tab in link dialog is still visible, also Font, Styles Format and color are available. This feature (integration) must be fixed soon as it may imply a very weird UX impression.

• plugins/image2/samples/assets/audi.jpg
• plugins/image2/samples/assets/maserati.jpg
  • We need licenses for these images, or new ones.

Done.

• plugins/image2/samples/contents.css
  • Missing copyright notice.

Done.

• The “h2” entry there is not required and can just cause confusion.

Done: Removed it.

• plugins/image2/samples/image2.html
  • The sample page is not a test page. It must simply illustrate the feature:
  • There is no need to have a framed and a inline editor. Only one (framed) is enough.
  • Complicated internal stuff, like “upcasting”, should not be used in the sample text. E.g. “I'm centered while upcasted!”.

Done: I simplified the page. Also I put a very basic description there.

[Olek Nowodziński]

Replying to matti:

Thanks looks awesome.

Awesome! ;)

With image2 plugin non-captioned image margins are handled with styles selection or is image2 plugin just widget feature demo?

Image2 is both a new feature of the editor and a kind of widget showcase. I'm not quite sure which styles you're talking about though. Could you give us some insight into your problem?

Cannot make a link from an image. I know bad idea, but you know how customers are :)

Still working on that. Sorry for inconvenience.

If you can bake all this in Image2 it would be awesome, but until you do I'm not so sure about replacing the old one yet.

The idea is to do it smoothly. At the moment it's not advised but with time as the widget gets more and more stable, the transition should be easy and painless.

[Piotrek Koszuliński]

Some more comments:

1. In the pluginDef.onLoad CKEDITOR.tools.cssVendorPrefix may be used.
2. The command 'image2' neither has allowedContent/requiredContent defined nor reacts when entering nested editables (it should be disabled like the rest of widgets commands). Note: if after beta we'll decide to merge both (block and inline) widgets, we will be able to remove it and use the default one, so all this will be fixed.
3. There's now better way to pass some data from upcast method to a widget then by attribute (​https://github.com/cksource/ckeditor-dev/blob/t/10659/plugins/image2/plugin.js#L536). You can just set data from upcast method - it is passed as a 2nd argument.
4. ​https://github.com/cksource/ckeditor-dev/blob/t/10659/plugins/image2/plugin.js#L613 - use tools.isEmpty instead. Faster, cleaner, stronger, better ;)

None of these block beta, but 1,3,4 can be easily fixed.

[Piotrek Koszuliński]

I also noticed that CKEDITOR.plugins.image2 lacks documentation.

[Matti Järvinen]

Replying to a.nowodzinski:

Replying to matti:

With image2 plugin non-captioned image margins are handled with styles selection or is image2 plugin just widget feature demo?

Image2 is both a new feature of the editor and a kind of widget showcase. I'm not quite sure which styles you're talking about though. Could you give us some insight into your problem?

Sorry, I can be unclear at times.

1. Open widget image2 sample
2. Use framed sample editor
3. Open Saturn V image properties
4. Change Saturn V image to align left
5. Uncheck captioned image

How to add spacing ( margin ) to the image so that it doesn't touch text?

Old image plugin ​http://ckeditor.com/demo has two options:

• "Styled image (left)" and "Styled image (right)" object styles
• HSpace and VSpace inputs on image properties dialog

Image2 plugin currently has none.

[Wim Leers]

I'd say margins don't belong in the content layer (the HTML), but in the presentation layer (the CSS).

But: my POV is different than yours, I think.

[Piotrek Koszuliński]

How to add spacing ( margin ) to the image so that it doesn't touch text?

Using CSS. We totally share Wim Leers' POV. The margins, borders, etc - it all belongs to the presentation layer which is managed by CSS, not HTML. Such options (v/hspacing and borders) should be never implemented in image dialog. We have them in the old one, but it was a mistake made many years ago and now we're fixing this.

[Olek Nowodziński]

Replying to Reinmar:

Some more comments:

1. In the pluginDef.onLoad CKEDITOR.tools.cssVendorPrefix may be used.

Done.

2. The command 'image2' neither has allowedContent/requiredContent defined nor reacts when entering nested editables (it should be disabled like the rest of widgets commands). Note: if after beta we'll decide to merge both (block and inline) widgets, we will be able to remove it and use the default one, so all this will be fixed.

I'm leaving this one for further development. OK?

3. There's now better way to pass some data from upcast method to a widget then by attribute (​https://github.com/cksource/ckeditor-dev/blob/t/10659/plugins/image2/plugin.js#L536). You can just set data from upcast method - it is passed as a 2nd argument.

Same as above.

4. ​https://github.com/cksource/ckeditor-dev/blob/t/10659/plugins/image2/plugin.js#L613 - use tools.isEmpty instead. Faster, cleaner, stronger, better ;)

Done.

I also added the Image2 to the index, corrected meta tags, title and description of the sample. Both branches are rebased on major.

[Piotrek Koszuliński]

Bravo!

[Olek Nowodziński]

Merged into major:

• dev ​git:1895ef4
• tests d2c9216

[Matti Järvinen]

Replying to Reinmar:

How to add spacing ( margin ) to the image so that it doesn't touch text?

Using CSS. We totally share Wim Leers' POV. The margins, borders, etc - it all belongs to the presentation layer which is managed by CSS, not HTML. Such options (v/hspacing and borders) should be never implemented in image dialog. We have them in the old one, but it was a mistake made many years ago and now we're fixing this.

What I was after is that there are no "Styled image (left)" and "Styled image (right)" object styles for plain image (not captioned one) available (at least in the sample).

Can I define object styles for Image2 widget's plain image?

[Piotrek Koszuliński]

It shouldn't be possible at this moment to define a style applicable to widgets, because styles system is not ready for this. Styles should not modify widgets, because they are non-editable entities. Widgets handle their structure and styling by themselves and it shouldn't be possible from "outside" to change them. So even if it is now possible to apply style to a widget, we're going to switch this off.

[Piotrek Koszuliński]

However, we could extend styles system to integrate it with widgets. But we have to think first how this should work. One of my ideas is that styles in this case would only modify widget's data, not DOM directly. Then widget would still handle its view internally.