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

t/135 Removed "vanilla" Lark #139

Merged
merged 19 commits into from
Feb 16, 2018
Merged

t/135 Removed "vanilla" Lark #139

merged 19 commits into from
Feb 16, 2018

Conversation

dkonopka
Copy link
Contributor

@dkonopka dkonopka commented Feb 15, 2018

Suggested merge commit message (convention)

Other: Removed styles for ck-editor-toolbar. Closes ckeditor/ckeditor5#3395.
Other: Removed border from ck-button class.
Tests: Wrapped buttons and icons in ToolbarView in manual theme test.

@dkonopka dkonopka requested a review from oleq February 15, 2018 14:50
@dkonopka dkonopka changed the title t/135 Removed "vanila" Lark t/135 Removed "vanilla" Lark Feb 15, 2018
@oleq
Copy link
Member

oleq commented Feb 15, 2018

Some impressions and bugs:

  • I'd get rid of these three variables too
    --ck-color-editor-border: var(--ck-color-base-border);
    --ck-color-editor-toolbar-background: var(--ck-color-base-foreground);
    --ck-color-editor-dropdown-background: var(--ck-color-base-background);
    . The point of this refac is to remove the middle (vanilla) layer completely. Default styles (colors) should consider the editor too.
  • I'd wrap all the buttons in the lark MT in .ck-toolbar. We can do that by code, creating a ToolbarViews to speed it up. ATM it's hard to tell what's going on
    image

@oleq
Copy link
Member

oleq commented Feb 15, 2018

The balloon should not have the same background as the toolbar
image

@dkonopka
Copy link
Contributor Author

dkonopka commented Feb 16, 2018

Ok, variables removed, manual test updated, I've added ToolbarView for every button/icon group.

@oleq Let's talk about balloon background, why (and when? :D) we decided to use two different backgrounds for toolbar and balloon? Currently, it looks really weird, maybe we should consider one for every panel in the editor?

toolbar bg - hsl(0, 0%, 98%)

screen shot 2018-02-16 at 09 57 06

balloon bg - hsl(0, 0%, 100%)

screen shot 2018-02-16 at 09 57 12

@oleq
Copy link
Member

oleq commented Feb 16, 2018

We never considered a balloon with a darker background when working on ckeditor/ckeditor5#645. IMO white balloons provide a better contrast between the wall of edited text and the UI and that's a good enough reason. It's more evident when in the context

screen shot 2018-02-16 at 10 40 10

screen shot 2018-02-16 at 10 40 46

@oleq
Copy link
Member

oleq commented Feb 16, 2018

Anyway, we can discuss the topic in a separate issue. Let's focus on the task at hand and not touch the design we came up with.

@dkonopka
Copy link
Contributor Author

dkonopka commented Feb 16, 2018

Agree. I'll make follow-up then and bring back white background for the balloon.

edit: you were faster :D

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.

Does maintaining a vanilla and editor Lark in parallel make any sense?
2 participants