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

i/5806: Rounded corners should work for dropdown panel children in all positions #254

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

i/5806: Rounded corners should work for dropdown panel children in all positions #254

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a64f145
Fix: Rounded corners should work for dropdown panel children in all p…
oleq Nov 26, 2019
2988d74
Merge branch 'master' into i/5806
oleq Apr 9, 2020
e8a484e
Adjusted rounded corners in a list dropdown, split button and font co…
oleq Apr 9, 2020
7ae04dc
Merge branch 'master' into i/5806
oleq Apr 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions theme/ckeditor5-font/fontcolor.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

.ck .ck-button.ck-color-table__remove-color {
padding: calc(var(--ck-spacing-standard) / 2 ) var(--ck-spacing-standard);
border-top-left-radius: 0;
border-bottom-left-radius: 0;
border-bottom-right-radius: 0;

Expand Down
59 changes: 45 additions & 14 deletions theme/ckeditor5-ui/components/dropdown/listdropdown.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,59 @@

@import "../../../mixins/_rounded.css";

.ck.ck-dropdown .ck-dropdown__panel .ck-list {
/* Disabled radius of top-left border to be consistent with .dropdown__button
https://github.com/ckeditor/ckeditor5/issues/816 */
@mixin ck-rounded-corners {
border-top-left-radius: 0;
.ck.ck-dropdown .ck-dropdown__panel .ck-list .ck-list__item .ck-button {
border-radius: 0;
}

.ck.ck-dropdown .ck-dropdown__panel {
&.ck-dropdown__panel_sw > .ck-list .ck-list__item:first-child .ck-button {
@mixin ck-rounded-corners {
border-top-right-radius: 0;
border-bottom-right-radius: 0;
border-bottom-left-radius: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like it... Using this mixin here, repeatedly, makes the code too long and hard to read IMHO.
I think we should create a similar mixin but for reseting radiuses. Then we can mix it with ck-rounded-corners but also use it here to reset those and apply radius only for the one corner. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is what the mixin looks like https://github.com/ckeditor/ckeditor5-theme-lark/blob/0115e9440ce152135164ba4e4b6ad831b56ab6fd/theme/mixins/_rounded.css#L11-L20

All it does it border-radius: var(--ck-border-radius); and then everything from the brackets. It works as an exclusion.

A mixin for resetting radiuses would... look exactly the same as @mixin ck-rounded-corners I guess. So I'm not sure we would gain that much code. postcss-mixins does not support conditionals so there's no way to specify which radiuses should be reset.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I know how the mixin looks like, checked it before ;-) So basically:

@mixin ck-rounded-corners {
	border-top-right-radius: 0;
	border-bottom-right-radius: 0;
	border-bottom-left-radius: 0;
}

does the same thing as:

border-radius: 0;
border-top-left-radius: var(--ck-border-radius);

right?

The latter is much simpler to me, so my suggestion is to apply the code above or introduce:

@mixin ck-reset-corners {
    border-top-left-radius: var(--ck-border-radius);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

It makes sense but honestly, I'm confused by the naming. ck-rounded-corners enables rounded corners then you can opt-out from some of them. ck-reset-corners also adds rounded corners but it does not make sense standalone without @mixin-content.

Maybe we should investigate and see if there's a chance for us to write mixins in JS. We could have a better rounded-corners mixin then:

/* Enable all */
@mixin ck-rounded-corners;

/* Enable bottom-right and top-left only */
@mixin ck-rounded-corners bottom-right top-left;

/* Do something custom (it only adds selectors) */
@mixin ck-rounded-corners {
    border-top-left-radius: 2cm;
}

I'm not sure how this could work with our ultra-modular framework. That's probably a follow-up.

Copy link
Contributor

Choose a reason for hiding this comment

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

I get your point here. So to wrap it all up, you still prefer this:

@mixin ck-rounded-corners {
	border-top-right-radius: 0;
	border-bottom-right-radius: 0;
	border-bottom-left-radius: 0;
}

over this:

border-radius: 0;
border-top-left-radius: var(--ck-border-radius);

?

}
}

&.ck-dropdown__panel_se > .ck-list .ck-list__item:first-child .ck-button {
@mixin ck-rounded-corners {
border-top-left-radius: 0;
border-bottom-right-radius: 0;
border-bottom-left-radius: 0;
}
}

/* Make sure the button belonging to the first/last child of the list goes well with the
border radius of the entire panel. */
& .ck-list__item {
&:first-child .ck-button {
&.ck-dropdown__panel_sw,
&.ck-dropdown__panel_se {
& > .ck-list .ck-list__item:last-child .ck-button {
@mixin ck-rounded-corners {
border-top-right-radius: 0;
border-top-left-radius: 0;
border-bottom-left-radius: 0;
border-bottom-right-radius: 0;
}
}
}

&.ck-dropdown__panel_ne > .ck-list .ck-list__item:last-child .ck-button {
@mixin ck-rounded-corners {
border-bottom-left-radius: 0;
border-top-right-radius: 0;
border-top-left-radius: 0;
}
}

&.ck-dropdown__panel_nw > .ck-list .ck-list__item:last-child .ck-button {
@mixin ck-rounded-corners {
border-bottom-right-radius: 0;
border-top-right-radius: 0;
border-top-left-radius: 0;
}
}

&:last-child .ck-button {
&.ck-dropdown__panel_nw,
&.ck-dropdown__panel_ne {
& > .ck-list .ck-list__item:last-child .ck-button {
@mixin ck-rounded-corners {
border-top-left-radius: 0;
border-top-right-radius: 0;
border-bottom-right-radius: 0;
border-bottom-left-radius: 0;
}
}
}
Expand Down
26 changes: 19 additions & 7 deletions theme/ckeditor5-ui/components/dropdown/splitbutton.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,17 @@

@nest [dir="ltr"] & {
/* Don't round the arrow button on the left side */
@mixin ck-rounded-corners {
border-top-left-radius: unset;
border-bottom-left-radius: unset;
}
border-top-left-radius: unset;
border-top-right-radius: var(--ck-border-radius);
border-bottom-left-radius: unset;
border-bottom-right-radius: var(--ck-border-radius);
}

@nest [dir="rtl"] & {
/* Don't round the arrow button on the right side */
border-top-left-radius: var(--ck-border-radius);
border-top-right-radius: unset;
border-bottom-left-radius: var(--ck-border-radius);
border-bottom-right-radius: unset;
}

Expand Down Expand Up @@ -78,14 +80,24 @@
/* Don't round the bottom left and right corners of the buttons when "open"
https://github.com/ckeditor/ckeditor5/issues/816 */
&.ck-splitbutton_open {
@mixin ck-rounded-corners {
& > .ck-splitbutton__action {
& > .ck-splitbutton__action {
@nest [dir="ltr"] & {
border-bottom-left-radius: 0;
}

& > .ck-splitbutton__arrow {
@nest [dir="rtl"] & {
border-bottom-right-radius: 0;
}
}

& > .ck-splitbutton__arrow {
@nest [dir="ltr"] & {
border-bottom-right-radius: 0;
}

@nest [dir="rtl"] & {
border-bottom-left-radius: 0;
}
}
}
}
32 changes: 30 additions & 2 deletions theme/ckeditor5-ui/components/dropdown/toolbardropdown.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,34 @@
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
*/

.ck.ck-toolbar-dropdown .ck-toolbar {
border: 0;
@import "../../../mixins/_rounded.css";

.ck.ck-toolbar-dropdown .ck-dropdown__panel {
& > .ck-toolbar {
border: 0;
}

&.ck-dropdown__panel_sw > .ck-toolbar {
@mixin ck-rounded-corners {
border-top-right-radius: 0;
}
}

&.ck-dropdown__panel_se > .ck-toolbar {
@mixin ck-rounded-corners {
border-top-left-radius: 0;
}
}

&.ck-dropdown__panel_ne > .ck-toolbar {
@mixin ck-rounded-corners {
border-bottom-left-radius: 0;
}
}

&.ck-dropdown__panel_nw > .ck-toolbar {
@mixin ck-rounded-corners {
border-bottom-right-radius: 0;
}
}
}