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

T/ckeditor5/645: Updated UI components to bring the support for the refreshed Lark #362

Merged
merged 19 commits into from
Feb 2, 2018

Conversation

oleq
Copy link
Member

@oleq oleq commented Jan 31, 2018

Suggested merge commit message (convention)

Feature: Updated UI components to bring the support for the refreshed Lark theme (see ckeditor/ckeditor5#645).

BREAKING CHANGE: The DOM structure of the dropdown component has changed. Please refer to the documentation to find out more.

BREAKING CHANGE: Basic properties of the balloon panel component have changed (i.e. the location of the arrow, the default positions), which may have an impact on existing integrations.


Additional information

@oleq oleq requested a review from dkonopka January 31, 2018 15:15
pointer-events: none;
z-index: var(--ck-z-default);

position: absolute;
top: 50%;
transform: translate3d( 0, -50%, 0 );
top: 52%;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering why are we using here some "weird" numbers (52%, 55%)? Is the reason not perfectly centred align icons? As I see in the manual test there is no big difference with previous values and I made a little comparison, with 52% top dropdown icon fits not correctly to button box-size.

dropdown-comparision

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess 52 vs. 55 is a bug. They should probably be the same.

But my intention was to make the arrow centered in terms of perception, not geometry. The arrow gravitates visually to the top so moving it down a little bit makes the alignment more appealing IMO.

:root {
--ck-dropdown-icon-size: 10px;
}

.ck-dropdown {
display: inline-block;
position: relative;

/* A triangle displayed to indicate this is actually a dropdown. */
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we can remove comment here, .ck-dropdown__arrow explains everything.

@@ -5,21 +5,27 @@

@import "../tooltip/mixins/_tooltip.css";

:root {
--ck-dropdown-icon-size: 10px;
Copy link
Contributor

Choose a reason for hiding this comment

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

We need here em values. Let's change -ck-font-size-base to 40px:

screen shot 2018-02-01 at 10 09 42

Changing it to var(--ck-font-size-tiny) works well.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 9e85ec4 on t/ckeditor5/645 into 3175a64 on master.

@Reinmar Reinmar merged commit 623d536 into master Feb 2, 2018
@Reinmar Reinmar deleted the t/ckeditor5/645 branch February 2, 2018 13:26
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