t/ckeditor5-ui/144: Migrated the package from SASS to PostCSS #114

oleq · 2017-11-22T13:48:26Z

Suggested merge commit message (convention)

Other: Migrated the theme from SASS to PostCSS (see ckeditor/ckeditor5#5304).

BREAKING CHANGE: The styles are no longer developed in SASS which means various .scss files (including variables, mixins, etc.) became unavailable. Please refer to the Theme Customization guide to learn more about migration to PostCSS.

This reverts commit 215cebe.

dkonopka · 2017-11-23T08:25:56Z

theme/ckeditor5-ui/globals/_reset.css

+ * For licensing, see LICENSE.md.
+ */
+
+@custom-selector :--ck-reset-all


It's the only one case of used @custom-selector postcss plugin - getpostcssconfig.js in all stylesheets, do we really need it?

A good point. Probably it's better to get rid of it.

dkonopka · 2017-11-23T09:10:35Z

theme/ckeditor5-ui/globals/_reset.css

+@custom-selector :--ck-reset-all
+	.ck-reset_all,
+	.ck-reset_all *,
+	.ck-reset_all a,


Is it necessary here to use 1-1 CSS specificity for <a> and <textarea> elements?

As we talked it was an issue with CK4 (IE10 maybe?) because UA override, but as I checked Chrome and Firefox and there is no override situation.

Probably it's a legacy code because I copied it straight from v4.

dkonopka · 2017-11-23T10:52:25Z

theme/ckeditor5-engine/placeholder.css

+
+.ck-placeholder::before {
+	cursor: text;
+	color: #c2c2c2;


Maybe we should use a variable here?

dkonopka · 2017-11-23T11:17:29Z

theme/ckeditor5-ui/components/icon.css

+
+svg.ck-icon {
+	width: calc(var(--ck-line-height-base) * 1em);
+	height: calc(var(--ck-line-height-base) * 1em);


I see --ck-font-size-normal: 1em, is it worthy to change it?

What do you mean?

I was thinking about changing 1em to variable:

width: calc(var(--ck-line-height-base) * var(--ck-font-size-normal));

dkonopka · 2017-11-23T11:20:30Z

theme/ckeditor5-ui/components/inputtext.css

+	background: var(--ck-color-input-background);
+	border: 1px solid var(--ck-color-input-border);
+	padding: var(--ck-spacing-small) var(--ck-spacing-medium);
+	min-width: 21em;


Like earlier: maybe variable for min-width?

dkonopka · 2017-11-23T11:34:25Z

theme/ckeditor5-ui/components/tooltip/tooltip.css

+	 */
+	&.ck-tooltip_s {
+		bottom: calc(-1 * var(--ck-tooltip-arrow-size));
+		transform: translateY( 100% );


Let's speed up that transform: with translate3d( 0, 100%, 0);

I wouldn't be so sure about it. It needs to be checked because I experienced some issues in the past developing tooltips.

Thinking again and you are right this is about tooltip "triangle" and maybe it isn't worth to use hardware acceleration (GPU) caused by will-change/translate3d property.

Anyway, we need to remember that sometimes we will need to use will-change/translate3d - see this issue with firefox blurred icons.

dkonopka · 2017-11-23T11:34:57Z

theme/ckeditor5-ui/components/tooltip/tooltip.css

+
+		& .ck-tooltip__text::after {
+			top: calc(-1 * var(--ck-tooltip-arrow-size));
+			transform: translateX( -50% );


Let's speed up that transform: with translate3d( -50%, 0, 0);

👉 #114 (comment)

dkonopka · 2017-11-23T11:35:52Z

theme/ckeditor5-ui/components/tooltip/tooltip.css

+	 */
+	&.ck-tooltip_n {
+		top: calc(-1 * var(--ck-tooltip-arrow-size));
+		transform: translateY( -100% );


-> translate3d.

👉 #114 (comment)

dkonopka · 2017-11-23T11:43:51Z

theme/ckeditor5-ui/globals/_fonts.css

+	--ck-font-size-normal: 1em;
+	--ck-font-size-big: 1.4em;
+	--ck-font-size-large: 1.8em;
+}


We are setting levels of property with calc in theme/ckeditor5-ui/globals/_spacing.css maybe we should use it here?

Probably a good idea.

OTTH I noticed that while spacings multiply .8em, which is not super-easy, fonts are related to 1em, which is pretty straightforward. Changing it to calc would be

--ck-font-size-big: 1.4em;

vs

--ck-font-size-big: calc(var(--some-base-size) * 1.4));

which looks bloated. I don't think it's a good idea.

dkonopka · 2017-11-23T12:25:51Z

theme/ckeditor5-ui/components/tooltip/tooltip.css

@@ -0,0 +1,82 @@
+/*


Why did you decide to make a folder for tooltip.css instead of placing it directly into /components?

Is there any reason? Because I see e.g: /components/panel/balloonpanel.css & /components/panel/stickypanel.css and understand that it needs a folder.

Check out the structure of theme in ckeditor5-ui. AFAIR the tooltip has a mixin and thus it has been defined as a directory (multiple files). The theme must follow such pattern if it is to be loaded properly.

TBH, now that I think about it, I think each component in ckeditor5-ui should have its own directory because if one day it gets a mixin or whatever, we wouldn't need to fix a structure of N–themes to reflect ckeditor5-ui and make them work with the theme importer. If you know what I mean :P

Ok, I see this and it's clear to me 👍

dkonopka · 2017-11-23T12:30:27Z

theme/ckeditor5-link/linkform.css

+	padding: var(--ck-spacing-large);
+
+	&:focus {
+		/*https://github.com/ckeditor/ckeditor5-link/issues/90*/


Lack of space after /* and before */ + line 28 & 35.

dkonopka · 2017-11-23T12:51:37Z

theme/ckeditor5-block-quote/blockquote.css

+ */
+
+blockquote {
+	border-left: solid 5px #CCC;


Like in code.css we can use variable here too: --ck-color-blockquote-border.

dkonopka · 2017-11-23T13:00:38Z

theme/ckeditor5-image/image.css

+		text-align: center;
+
+		& > img {
+			/*Center the image if its width is smaller than content's width.*/


Missing spaces in the comment.

dkonopka · 2017-11-23T13:03:55Z

theme/ckeditor5-image/imagestyle.css

+	& .image-style-align-left,
+	& .image-style-align-center,
+	& .image-style-align-right {
+		max-width: var(--ck-image-max-width);


I'm not sure but for center aligned imagemax-width should be 50%?

Check out the current code in SASS. I think I just copy–pasted it.

True, just wondering why centered style of image indicate max-width: 50% 🤔

dkonopka · 2017-11-24T08:56:52Z

theme/ckeditor5-ui/components/panel/balloonpanel.css

+
+	&.ck-balloon-panel_with-arrow {
+		&:before,
+		&:after {


We should use a double-colon instead of one for pseudoelements.

dkonopka · 2017-11-24T08:58:01Z

theme/ckeditor5-ui/components/editorui.css

+
+/* https://github.com/ckeditor/ckeditor5-theme-lark/issues/111 */
+.ck-toolbar-container.ck-editor-toolbar-container[class*="arrow_n"] {
+	&:after {


Missing double-colon here and line 65 too.

dkonopka · 2017-11-24T10:21:21Z

theme/ckeditor5-upload/imageuploadprogress.css

+	& .ck-progress-bar {
+		height: 2px;
+		width: 0;
+		background: #6ab5f9;


Would be nice to make variable here --ck-image-upload-progressbar-line-background

dkonopka · 2017-11-24T10:22:25Z

theme/ckeditor5-upload/imageuploadprogress.css

+	&.ck-infinite-progress::before {
+		width: var(--ck-image-upload-progress-line-width);
+		height: 2px;
+		background: rgba(0, 0, 0, 0.1);


Would be nice to make variable here --ck-image-upload-progress-line-background

dkonopka · 2017-11-24T10:32:30Z

theme/ckeditor5-heading/heading.css

+ */
+
+.ck-heading_heading1 {
+	font-size: 1.5em;


What do you think to use here variable (--ck-font-size-normal: 1em;) and make it:

font-size: calc(var(--ck-font-size-normal) * 1.5);

and apply it to heading2 and heading3 too?

dkonopka · 2017-11-24T10:58:30Z

theme/ckeditor5-editor-classic/classiceditor.css

+				border-bottom-right-radius: 0;
+			}
+
+			/* https://github.com/ckeditor/ckeditor5-editor-classic/issues/62 */


Something is wrong with that url (I mean it is not that issue).

dkonopka · 2017-11-27T09:19:14Z

docs/_snippets/examples/custom.css

+
+	--ck-color-button-action-background: #1ABC9C;
+	--ck-color-button-action-border: #49d2b7;
+	--ck-color-button-action-focus-background: #17a98c;


As we talked we can't use postcss plugin for colors with functions like darken/lighten so maybe should we use hsl format? It be easier to understand & modify shades of color.

--ck-color-button-action-background: hsl( 168, 76%, 42% ) --ck-color-button-action-focus-background: hsl( 168, 76%, 38% )

That's a great idea. And what's more, you can use variables and calc to shift the color.

:root { --lightness-shift: 30%; } #first { background: hsl(120,100%, 50%); } #second { background: hsl(120,100%, calc(50% - var(--lightness-shift))); }

...which may be broken in Firefox (and IE11?). Needs research.

Perfect solution! I forgot about changing the shade of colour by a variable in HSL.

HSL is full supported since IE8+ so no worries here.

And we don't need any postcss color plugin 🎉

Pity we won't be able to use calc() with var(). I checked that out, it does not work in FF and IE11.

Still, hsl sounds like a plan.

…idth.

…les sheets.

…migration in the future.

oleq added 9 commits November 7, 2017 17:18

Migrated UI components from SASS to PostCSS. Added theme support.

55fe28a

Refactored theme structure for easier loading.

76de100

Removed color() calls in the styles. Code refactoring.

ffe7946

Removed redundant color variables.

215cebe

Merge branch 'master' into t/ckeditor5-ui/144

5044a47

Revert "Removed redundant color variables."

af1e48f

This reverts commit 215cebe.

Docs: Updated custom theme example to the new theme architecture.

faa4a35

Code refactoring in colors.

5d25033

Docs: Updated Theme customization guide to work with PostCSS.

4461a30

oleq requested review from Reinmar and dkonopka November 22, 2017 13:48

oleq mentioned this pull request Nov 22, 2017

T/144: Migrated the package from SASS to PostCSS ckeditor/ckeditor5-ui#345

Merged

dkonopka reviewed Nov 23, 2017

View reviewed changes

dkonopka reviewed Nov 24, 2017

View reviewed changes

dkonopka reviewed Nov 27, 2017

View reviewed changes

oleq added 12 commits November 29, 2017 13:44

Got rid of postcss-custom-selectors to simplify the building process.

d462ecd

Removed obsolete 'a' and 'textarea' selectors from .ck-reset_all.

06b8b23

Created variable for engine's placeholder text color.

0047c44

Used --ck-font-size-normal variable to define .ck-icon's height and w…

8c32d11

…idth.

Defined a variable to control .ck-input-text's min-width.

b5a7b45

Code refactoring in the linkform styles sheet.

bcbbe35

Created variables for block–quote–in–content styles.

37ef96d

Code refactoring: added missing spaces to docs strings in various sty…

53d6d04

…les sheets.

Code refactoring: added missing colons for pseudo-element selectors.

5ba4672

Defined variables for colors in the upload styles sheet.

4226d5c

Used variables to define .ck-heading_headingN font sizes.

2bd4ac7

Removed obsolete comment from the classiceditor styles sheet.

0a74d03

dkonopka approved these changes Nov 29, 2017

View reviewed changes

oleq added 2 commits November 29, 2017 16:34

Converted colors to HSL(A).

041b540

Moved ckeditor5-ui component styles to own directories to avoid such …

7594cd2

…migration in the future.

oleq merged commit efc6004 into master Nov 30, 2017

oleq deleted the t/ckeditor5-ui/144 branch November 30, 2017 16:32

t/ckeditor5-ui/144: Migrated the package from SASS to PostCSS #114

t/ckeditor5-ui/144: Migrated the package from SASS to PostCSS #114

Conversation

oleq commented Nov 22, 2017 • edited by pomek Loading

Suggested merge commit message (convention)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkonopka Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkonopka Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkonopka Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkonopka Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkonopka Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkonopka Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

dkonopka Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

dkonopka Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkonopka Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleq commented Nov 22, 2017 •

edited by pomek

Loading

dkonopka Nov 24, 2017 •

edited

Loading

dkonopka Nov 24, 2017 •

edited

Loading

dkonopka Nov 23, 2017 •

edited

Loading

dkonopka Nov 23, 2017 •

edited

Loading

dkonopka Nov 24, 2017 •

edited

Loading

dkonopka Nov 24, 2017 •

edited

Loading

dkonopka Nov 24, 2017 •

edited

Loading

dkonopka Nov 27, 2017 •

edited

Loading

dkonopka Nov 29, 2017 •

edited

Loading