Opened 3 years ago

Closed 3 years ago

#13829 closed Bug (fixed)

Add class based on widget type to the widget wrapper

Reported by: Marek Lewandowski Owned by: Szymon Kupś
Priority: Normal Milestone: CKEditor 4.6.0
Component: General Version:
Keywords: Cc:

Description (last modified by Marek Lewandowski)

This is a follow-up of #13200 issue.

As @Reinmar writes:

Inability to write rules matching certain widget wrapper + widget class

Once classes will be moved to wrappers it would be good if it was possible to write rules like:

.image.centered { ... }
.embed.centered { ... }

As I experienced recently centered image's wrapper may need very different styles than that of an embed widget. E.g. image's wrapper must have text-align: center. This affects embed widgets which may be unwanted if we only want to use margin-left/right: auto.

Hence, it will be useful if widget wrapper had classes based on widgets names - e.g. "widget-image", "widget-embed", etc.

Change History (10)

comment:1 Changed 3 years ago by Marek Lewandowski

Status: newconfirmed

comment:2 Changed 3 years ago by Marek Lewandowski

Description: modified (diff)

comment:3 Changed 3 years ago by Marek Lewandowski

Let's use prefixed class names so we'd end up with classes like cke-widget-image, cke-widget-codesnippet etc.

comment:4 Changed 3 years ago by Szymon Kupś

Owner: set to Szymon Kupś
Status: confirmedassigned

comment:5 Changed 3 years ago by Szymon Kupś

Status: assignedreview

Pushed branch:t/13829. Added CSS class based on pattern cke_widget_<widgetname> to the widget wrapper.

comment:6 Changed 3 years ago by Marek Lewandowski

Status: reviewreview_failed

Your solution relied on widgetName param which (as doc says) is optional. This argument is not given e.g. in widget upcasting (happens when you paste a widget).

  1. Open a page with editor and any widget plugin.
  2. Add a widget.
  3. Move selection to the other place of the document.
  4. Type one letter.
  5. Press ctrl + z once (undo).

Expected: Your widget should keep the cke_widget_<name> class.

Expected: The class is removed.

Other case might include copy & paste or drag & dropping the widget.

I've added some changes to t/13829 branch, please check it and tell if my patch looks fine. If so change ticket status to review_passed, add a changelog entry and merge branch to branch (more info in docs).

Last edited 3 years ago by Marek Lewandowski (previous) (diff)

comment:7 Changed 3 years ago by Szymon Kupś

Status: review_failedassigned

comment:8 Changed 3 years ago by Szymon Kupś

Status: assignedreview

I've pushed one commit to branch:t/13829 - changed regular expression in widgetWrapperAtributes to match two tests in http://tests.ckeditor.dev:1030/tests/plugins/widget/widgetsintegration

comment:9 Changed 3 years ago by Tade0

Status: reviewreview_failed

That last commit indeed fixes these tests.

As for the modifications by @m.lewandowski: I tested how this works with copying/pasting, drag & drop, mode switching and the class was there each time.

As for the code: I have one issue that I believe deserves attention: https://github.com/cksource/ckeditor-dev/blob/t/13829/plugins/widget/plugin.js#L644

This condition is always true.

comment:10 Changed 3 years ago by Tade0

Resolution: fixed
Status: review_failedclosed

Fixed the always-true condition problem, since it's a minor thing.

Fixed with git:45f3e0e84afddbcfaeebe939a364a61bef905f15.

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