Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#13818 closed New Feature (fixed)

Allow to link widget style definitions, so applying one style might disable the other

Reported by: Marek Lewandowski Owned by: Wiktor Walc
Priority: Normal Milestone: CKEditor 4.6.2
Component: General Version: 4.4.0
Keywords: Cc:

Description

This is a follow-up of #13200 issue.

Some styles styles might exclude the others, take alignment class for an example. Having left-align and right-align class, we want to use only one at a time.

What we can do is to add group property to the style definition passed to config.stylesSet.

var stylesSet = [
    { name: 'Left align', type: 'widget', widget: 'image', attributes: { 'class': 'left-align' }, gorup: 'alignment' },
    { name: 'Right align', type: 'widget', widget: 'image', attributes: { 'class': 'right-align' }, gorup: 'alignment' }
];

Scope of the Fix

For the time being let's restrict only to widget system. Later on we might bring support for that for Object styles, that are implemented in core/style.js.

Multiple Groups

Now there possiblty might be a case when style fits (excludes) more than one group. So we should also accept array of groups in such case.

Change History (11)

comment:1 Changed 3 years ago by Marek Lewandowski

Status: newconfirmed

comment:2 Changed 3 years ago by Szymon Kupś

Owner: set to Szymon Kupś
Status: confirmedassigned

comment:3 Changed 3 years ago by Szymon Kupś

Status: assignedreview

Pushed branch:t/13818. I've added style groups handling to widget's custom style handler and introduced new method style.removeStylesFromSameGroup().
Also updated stylescombo plugin, checking for edge case when there are styles from multiple groups applied together.

comment:4 Changed 3 years ago by Tade0

Status: reviewreview_failed

After some digging I found a way to break something: Switch to source mode and apply conflicting styles this way(via adding classes), then switch back. The result is that the styles are still applied.

I've done some minor style corrections and added the sourcearea plugin to the manual test.

Changes pushed to branch:t/13818.

comment:5 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.0CKEditor 4.6.1

comment:6 Changed 2 years ago by Wiktor Walc

Owner: changed from Szymon Kupś to Wiktor Walc
Status: review_failedreview

Keep in mind that applying stupid operations in source mode can be considered as unsupported edge case. If this was the only reason to fail the review, please check it again, ignoring the case above.

comment:7 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.1CKEditor 4.6.2

comment:8 in reply to:  6 Changed 2 years ago by Marek Lewandowski

Replying to wwalc:

Keep in mind that applying stupid operations in source mode can be considered as unsupported edge case. If this was the only reason to fail the review, please check it again, ignoring the case above.

+1 on that

comment:9 Changed 2 years ago by kkrzton

Status: reviewreview_passed

Apart from the issue mentioned in comment:4 which we decided to skip as it is unsupported edge case, everything looks good.

comment:10 Changed 2 years ago by kkrzton

Resolution: fixed
Status: review_passedclosed

Merged to master with c300ed9.

comment:11 Changed 2 years ago by Marek Lewandowski

Type: BugNew Feature

It's actually a new feature rather than a bug fix.

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