Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Command for printing content styles #1805

Merged
merged 20 commits into from
Aug 14, 2019
Merged

Command for printing content styles #1805

merged 20 commits into from
Aug 14, 2019

Conversation

pomek
Copy link
Member

@pomek pomek commented Jun 10, 2019

Suggested merge commit message (convention)

Other: Added an npm script docs:content-styles which build an editor with all plugins and save all occurrence of.ck-content class into a separate CSS file. Closes #773.

@pomek pomek requested a review from Reinmar June 10, 2019 10:07
Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

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

Two issues I found so far when I ran the task:

Variables

This piece of CSS does not make sense without variable declarations:

.ck-content .image-style-align-right {
	float: right;
	margin-left: var(--ck-image-style-spacing);
}

we must figure out how to include them in the output content styles.

Comments

Comments in the output should not contain the path to ckeditor5 (root). The prefix does not make sense for developers when the content-styles.css is uploaded to the server (production).

/* /Users/oleq/ck/5/ckeditor5/packages/ckeditor5-image/theme/image.css */
.ck-content .image {
	display: table;
	clear: both;
	text-align: center;
	margin: 1em auto
}

should be

/* ckeditor5/packages/ckeditor5-image/theme/image.css */
.ck-content .image {
	display: table;
	clear: both;
	text-align: center;
	margin: 1em auto
}

@pomek
Copy link
Member Author

pomek commented Jul 26, 2019

@oleq, could you look on this once again?

Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

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

ATM the script outputs custom properties that do not concern content styles. For instance

	--ck-color-button-default-background: transparent;
	--ck-color-button-default-hover-background: hsl(0, 0%, 90%);
	--ck-color-button-default-active-background: hsl(0, 0%, 85%);
	--ck-color-button-default-active-shadow: hsl(0, 0%, 75%);

these are UI–related custom properties and have no value in the context of content styles. They are just noise and they bloat the code.

Some way to include only those variables that are related to .ck-content must be found. Since we already know the full list I guess the best option would be collecting .ck-content ... rules that use custom properties in PostCSS and outputting an intersection between the full list and these few relevant cases.

@pomek
Copy link
Member Author

pomek commented Jul 29, 2019

@oleq, fixed.

Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

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

  1. I got

    --ck-color-upload-bar-background: hsl(209, 92%, 70%);

    but it's not used anywhere in the content styles. Wondering where did it come from?

  2. Also, let's ditch the ckeditor5/packages/ prefix from the comments maybe. It does not say much, I suppose.

  3. A header would be awesome so developers know what this file is (they're gonna upload it to their servers) and whether it is outdated or not:

    /*
     * CKEditor 5 (v12.3.1) content styles.
     * Generated on Tue Jul 30 15:36:18 CEST 2019.
     * For more information, check out https://ckeditor.com/docs/ckeditor5/latest...
     */
  4. We need the documentation.

    • We're gonna explain how to generate these styles, what they are (or not) and what to do with them. It could be in the "Integration" section of the "Builds". Or as a tutorial in the "Framework"
    • We can link to the page in the header.

@pomek
Copy link
Member Author

pomek commented Jul 30, 2019

image

@pomek
Copy link
Member Author

pomek commented Jul 31, 2019

@oleq oleq mentioned this pull request Aug 1, 2019
@oleq
Copy link
Member

oleq commented Aug 1, 2019

I posted some thought on the tool in #773 (comment).

@oleq
Copy link
Member

oleq commented Aug 6, 2019

@Reinmar Can you please take a look at the docs I created?

@Reinmar
Copy link
Member

Reinmar commented Aug 12, 2019

That's a very good start. I've got one proposal, though – we could move "By generating it using the yarn docs:content-styles script in the root package of the editor" to framework/guides/contributing/development-environment.html because it's mainly for us. I'm not sure why would anyone else be interested in that. In the "content styles" guide we may have a short link to that, but I don't think that even this is necessary.

Copy link
Member

@Reinmar Reinmar left a comment

Choose a reason for hiding this comment

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

As commented.

@Reinmar
Copy link
Member

Reinmar commented Aug 12, 2019

BTW, the other option after adding .ck-content to your container would be to replace .ck-content with something custom. It should be fairly straightforward and safe.

@Reinmar Reinmar merged commit 6330ffb into master Aug 14, 2019
@Reinmar Reinmar deleted the t/773 branch August 14, 2019 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Content styles
3 participants