Opened 10 months ago

Closed 4 months ago

Last modified 4 months ago

#14569 closed Task (fixed)

New CKEditor skin

Reported by: m.lewandowski Owned by: k.krzton
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 10 months ago by m.lewandowski

  • Status changed from new to confirmed
  • Summary changed from New skin to New CKEditor skin

comment:2 Changed 6 months ago by m.lewandowski

  • Milestone set to CKEditor 4.6.0

comment:3 Changed 6 months ago by k.krzton

  • Owner set to k.krzton
  • Status changed from confirmed to review

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 6 months ago by a.nowodzinski

  • Status changed from review to review_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 6 months ago by a.nowodzinski

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 6 months ago by k.krzton

@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 6 months ago by a.nowodzinski

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 6 months ago by a.nowodzinski

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 6 months ago by a.nowodzinski

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 6 months ago by a.nowodzinski (previous) (diff)

comment:10 Changed 6 months ago by k.krzton

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 6 months ago by a.nowodzinski

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 follow-up: Changed 6 months ago by k.krzton

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 6 months ago by a.nowodzinski

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 6 months ago by k.krzton

  • Status changed from review_failed to review

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 6 months ago by k.krzton

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

Last edited 6 months ago by k.krzton (previous) (diff)

comment:16 Changed 5 months ago by a.nowodzinski

  • Status changed from review to review_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 5 months ago by a.nowodzinski (previous) (diff)

comment:17 Changed 5 months ago by k.krzton

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 4 months ago by k.krzton

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 4 months ago by m.lewandowski

  • Resolution set to fixed
  • Status changed from review_passed to closed

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

comment:20 Changed 4 months ago by a.nowodzinski

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

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