Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#14569 closed Task (fixed)

New CKEditor skin

Reported by: Marek Lewandowski Owned by: kkrzton
Priority: Normal Milestone: CKEditor 4.6.0
Component: UI : Skins Version:
Keywords: Cc:

Description

We could refresh our skin a little bit.

Change History (20)

comment:1 Changed 2 years ago by Marek Lewandowski

Status: newconfirmed
Summary: New skinNew CKEditor skin

comment:2 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.0

comment:3 Changed 2 years ago by kkrzton

Owner: set to kkrzton
Status: confirmedreview

Changes in t/14569. Apart from adding all moono-lisa skin files, I also switched the default skin to moono-lisa, which means updating some moono files (skin.js, readme.md), adding icons folder so moono can be used in dev mode and also updating icons in plugin folder to ones used by a new skin.

I also added separate PR with changes in colorbutton plugin needed for the new default color palette.

comment:4 Changed 2 years ago by Olek Nowodziński

Status: reviewreview_failed

Because editor.skin is an alias to CKEDITOR.skinName, not the other way around (https://github.com/cksource/ckeditor-dev/blob/t/14569/core/editor.js#L387-L388), the dialog cover (https://github.com/cksource/ckeditor-dev/blob/t/14569/plugins/dialog/plugin.js#L2111) fails to work in Moono-Lisa (editor.config.skin is undefined).

Check the global editor sample/index.html.


I guess this sort of issue may also affect other places where moono-lisa–specific assertions are used:

comment:5 Changed 2 years ago by Olek Nowodziński

IMO template images must also be preserved in *.gif format (but with the new design) for smooth upgrade.

ATM, if developers update to 4.6.0 (they may still want to use Moono), templates they created using CKEDITOR.addTemplates will lose their images, because template1.gif has been replaced by template1.png.

WDYT?

comment:6 Changed 2 years ago by kkrzton

@a.nowodzinski As I understand correctly, what you mean is that checks like this https://github.com/cksource/ckeditor-dev/blob/t/14569/plugins/dialog/plugin.js#L2111 should use CKEDITOR.skinName instead of config.skin to work correctly in all cases (because for default skin editor.config.skin is undefined)?

As for the templates you are right, it may break the usage for custom images for some users. Using .gif with a new design is a much safer approach.

comment:7 Changed 2 years ago by Olek Nowodziński

It could be ( CKEDITOR.skinName || editor.config.skin ) – to be checked.

As for template images, we can use .png if they look better but .gif fallback must also be provided.

comment:8 Changed 2 years ago by Olek Nowodziński

As for samples/skin/

  • Don't you think that the features of samples/skin/normal.html belong here samples/old/uicolor.html?
  • samples/skin/inline.html is the same as http://ckeditor.dev/samples/old/inlineall.html?

comment:9 Changed 2 years ago by Olek Nowodziński

I see no support for balloonpanel plugin in moono-lisa (http://tests.ckeditor.dev:1030/tests/plugins/balloonpanel/manual/balloonpanel).

Did you check Accessibility Checker with moono-lisa?

Last edited 2 years ago by Olek Nowodziński (previous) (diff)

comment:10 Changed 2 years ago by kkrzton

I started wondering if samples/skin/ are actually needed as we have lots of samples in samples/old/. Maybe we should remove whole samples/skin/ folder as it was created only for the purpose of presenting specific editor toolbar configurations in different modes?

As for the balloonpanel it is a part of Accessibility Checker so it will be styled along with the rest of the a11ychecker plugin.

comment:11 Changed 2 years ago by Olek Nowodziński

I started wondering if samples/skin/ are actually needed as we have lots of samples in samples/old/. Maybe we should remove whole samples/skin/ folder as it was created only for the purpose of presenting specific editor toolbar configurations in different modes?

I guess so. Most of the features (RTL, inline, etc.) can be tested with old/ samples. Maybe except HC mode sample, but it can be enabled system–wide and editor will respect it. So, yep, ditch them.

As for the balloonpanel it is a part of Accessibility Checker so it will be styled along with the rest of the a11ychecker plugin.

I think it should be aligned to moono-lisa before 4.6.0 is released. It's the last piece and one of the best features of v4 line.

comment:12 Changed 2 years ago by kkrzton

I have created a separate task for balloonpanel and a11ychecker plugin skin adjustments: http://dev.ckeditor.com/ticket/14763.

comment:13 in reply to:  12 Changed 2 years ago by Olek Nowodziński

Replying to k.krzton:

I have created a separate task for balloonpanel and a11ychecker plugin skin adjustments: http://dev.ckeditor.com/ticket/14763.

Thanks, @k.krzton!

comment:14 Changed 2 years ago by kkrzton

Status: review_failedreview

Fixed with 3d35e39.

I added proper skin name checking, changed template images back to *.gif (no need for *.png support as those images are monochromatic so there is no visual difference between *.gif and *.png). I also removed samples/skin.

comment:15 Changed 2 years ago by kkrzton

I also created a separate task for WebSpellChecker plugin skin adjustments: http://dev.ckeditor.com/ticket/14790.

Last edited 2 years ago by kkrzton (previous) (diff)

comment:16 Changed 2 years ago by Olek Nowodziński

Status: reviewreview_passed

I reviewed the changeset since http://dev.ckeditor.com/ticket/14569#comment:11 and it looks great to me.

IMO it's time we brought the new skin to the community to get more feedback from developers who integrate CKEditor in their applications. If any, further issues should be fixed as followups.

Good job!

Last edited 2 years ago by Olek Nowodziński (previous) (diff)

comment:17 Changed 2 years ago by kkrzton

Thanks @a.nowodzinski.

Just as a reminder, there is also a small PR related to colorbutton plugin which should be merged along with this ticket.

comment:18 Changed 2 years ago by kkrzton

I added one small commit e75baa9 to the branch. It is one line of CSS adding white box-shadow like border. It was added to AC dialog to make contrast between green button and focus border accessible and while the same colors are used in other dialogs, those dialogs needed to be updated.

I suppose there is no need for re-reviewing this ticket as it is only a visual change which was already accepted.

comment:19 Changed 2 years ago by Marek Lewandowski

Resolution: fixed
Status: review_passedclosed

Fixed with git:f5bad7c6c1 (merged to major). Great job!

comment:20 Changed 2 years ago by Olek Nowodziński

Good job, guys! It's a huge progress for the project <3

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