Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#10659 closed Bug (fixed)

New image widget

Reported by: Olek Nowodziński Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.3 Beta
Component: General Version: 4.3 Beta
Keywords: Cc: wim.leers@…, matti.jarvinen@…

Description

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

Change History (20)

comment:1 Changed 6 years ago by Olek Nowodziński

Owner: set to Olek Nowodziński
Status: newassigned

comment:2 Changed 6 years ago by Wim Leers

Cc: wim.leers@… added

comment:3 Changed 6 years ago by Olek Nowodziński

Status: assignedreview

comment:4 Changed 6 years ago by Matti Järvinen

Cc: matti.jarvinen@… added

Couple of mock screens or test URL would be awesome.

comment:5 in reply to:  4 Changed 6 years ago by 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 ;)

comment:6 Changed 6 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

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!”.

comment:7 in reply to:  6 ; Changed 6 years ago by 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.

comment:8 in reply to:  6 Changed 6 years ago by Olek Nowodziński

Status: review_failedreview

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.

comment:9 in reply to:  7 ; Changed 6 years ago by 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.

comment:10 Changed 6 years ago by 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.

comment:11 Changed 6 years ago by Piotrek Koszuliński

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

comment:12 in reply to:  9 Changed 6 years ago by 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.

comment:13 Changed 6 years ago by 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.

comment:14 Changed 6 years ago by 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.

comment:15 in reply to:  10 Changed 6 years ago by Olek Nowodziński

Replying to Reinmar:

Some more comments:

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

Done.

  1. 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?

  1. 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.

  1. 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.

comment:16 Changed 6 years ago by Piotrek Koszuliński

Status: reviewreview_passed

Bravo!

comment:17 Changed 6 years ago by Olek Nowodziński

Resolution: fixed
Status: review_passedclosed

Merged into major:

comment:18 in reply to:  14 Changed 6 years ago by 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?

comment:19 Changed 6 years ago by 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.

comment:20 Changed 6 years ago by 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.

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy