Opened 17 months ago

Closed 11 months ago

Last modified 11 months 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 17 months ago by Marek Lewandowski

Status: newconfirmed
Summary: New skinNew CKEditor skin

comment:2 Changed 13 months ago by Marek Lewandowski

Milestone: CKEditor 4.6.0

comment:3 Changed 13 months 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 13 months 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 13 months 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 13 months 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 13 months 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 13 months 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 13 months 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 13 months ago by Olek Nowodziński (previous) (diff)

comment:10 Changed 13 months 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 13 months 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 13 months 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 13 months 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 13 months 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 13 months ago by kkrzton

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

Last edited 13 months ago by kkrzton (previous) (diff)

comment:16 Changed 12 months 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 12 months ago by Olek Nowodziński (previous) (diff)

comment:17 Changed 12 months 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 11 months 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 11 months ago by Marek Lewandowski

Resolution: fixed
Status: review_passedclosed

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

comment:20 Changed 11 months 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