Skip to content
This repository was archived by the owner on Jun 26, 2020. It is now read-only.

t/ckeditor5/1457: Implemented Font Color and Font Background Color features #25

Merged
merged 94 commits into from
Mar 29, 2019

Conversation

Mateusz Samsel and others added 30 commits March 5, 2019 16:48
…to be more accurate, add feedback for the user when color is selected.
@oleq oleq changed the title [WIP] Introduce font-color and font-background-color plugins Introduce font-color and font-background-color plugins Mar 28, 2019
@oleq oleq changed the title Introduce font-color and font-background-color plugins t/ckeditor5/1457: Implemented Font Color and Font Background Color features Mar 28, 2019
@jodator
Copy link
Contributor

jodator commented Mar 28, 2019

My 2 cents:

  • consider different keyboard navigation #26

  • white color has no focus outline on keyboard navigation:

    Peek 2019-03-28 18-53
    (the colors on buttons were only for testing purposes)

  • currently selected color has no focus outline on keyboard navigation:

    Peek 2019-03-28 18-54

  • I don't like FontBackgroundColor* name. For me FontBackground* would be enough :) But it's rather meh..

@jodator
Copy link
Contributor

jodator commented Mar 29, 2019

OK. I think I'm done with reading all that :D

@mlewand mlewand self-assigned this Mar 29, 2019
@mlewand
Copy link
Contributor

mlewand commented Mar 29, 2019

Corrections

The default value registers 15 colors:

changed to:

The default value registers following colors:

It's better not to mention too much details of value that is right below, that way we reduce thw chance that someone edits the listing alone (adds/removes some colors) without updating this particular line.

I have simplified manual test description (it was basically duplicating the editable contnet).

Issues

  • The "recent/document" colors feature to pick up pasted/preloaded colors is missing, but this should not be blocking this PR extracted to #28.
  • hasBorder property won't work properly, as here the assumption is that the skin will be bright (as it's applied to bright colors). It won't work without reconfiguration with dark themes.

minor:

  • There's no string value example in FontBackgroundColorConfig.colors (and respective font color struct).
  • FontBackgroundColorConfig and FontColorConfig implement identical interface and both have the same default values. It would improve DRY factor to have a common interface for both features. I see that now the listings differ in both, but having common interface should simply mention both configuration names.

Above things will be extracted to a separate issues not to block this PR.

@mlewand mlewand self-requested a review March 29, 2019 13:42
Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok all the things extracted, PR is ready to land on master branch. I'll merge it in few mins.

@mlewand mlewand merged commit c456b2a into master Mar 29, 2019
@mlewand mlewand deleted the t/ckeditor5/1457 branch March 29, 2019 14:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants