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

t/494: Increased the specificity of CSS rules. Introduced the .ck class for editor UI components #921

Merged
merged 8 commits into from
Mar 29, 2018

Conversation

@oleq oleq requested a review from dkonopka March 23, 2018 14:18
@Reinmar
Copy link
Member

Reinmar commented Mar 26, 2018

Why did you increase the specificity of content styles too? This issue was about UI's styles. Content styles are a different topic – they should be fairly simple to be overridden by the developer.

@oleq
Copy link
Member Author

oleq commented Mar 26, 2018

These are the documentation styles, not user content styles.

@Reinmar
Copy link
Member

Reinmar commented Mar 26, 2018

I didn't mean this. I meant all the changes in basic-stylers, block-quote, image, headings, etc.

@oleq
Copy link
Member Author

oleq commented Mar 26, 2018

Maybe you're right. WDYT @dkonopka?

example

@dkonopka
Copy link
Contributor

dkonopka commented Mar 26, 2018

I agree, blockquote border-color or code background should be easy to change, without fighting with CSS specificity, because we are skipping CSS Variables in content styles ( https://github.com/ckeditor/ckeditor5-block-quote/issues/21)

So I'm ok with adding extra .ck for UI Elements like buttons, icons, tooltips to be sure that it will be displayed properly in most cases and we have a lot of vars to change color/size/background of it.

@oleq
Copy link
Member Author

oleq commented Mar 28, 2018

I undid the changes in basic-styles, block-quote and image.

Copy link
Contributor

@dkonopka dkonopka left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@Reinmar
Copy link
Member

Reinmar commented Mar 29, 2018

I reviewed all the changes and I'm unsure about these:

/* See https://github.com/ckeditor/ckeditor5/issues/494 */
margin-left: 0;
}

/* examples/builds/inline-editor.html */
.live-snippet .image-style-left, .live-snippet .image-style-right {
Copy link
Member

Choose a reason for hiding this comment

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

These changes don't make much sense to me. Either .ck or .live-snippet. The latter was used so far to bump up the specificity. Now, one of these become unnecessary.

Copy link
Member

Choose a reason for hiding this comment

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

Also, .ck is part of the UI, and .image-style-left of the content. So, if anything, the selector which makes the most sense here is .ck-content .image-style-left. But then, the content will not look good before the editor is initialised, so it'd be good to use a class which is always present. Which is... .live-snippet ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's stick to .l-s :P

@oleq
Copy link
Member Author

oleq commented Mar 29, 2018

https://github.com/ckeditor/ckeditor5-heading/compare/t/ckeditor5/494 – I assume this styles the headings in the dropdown. I'm not entirely sure whether I think about this more as about the content or about the UI.

I consider it the UI.

https://github.com/ckeditor/ckeditor5-image/compare/t/ckeditor5/494#diff-1d370f46d756a5c592c29c39a45704fbL11 – I think we miss the figure part here. For some reason, the figure.image always come together for me. But maybe it's unnecessary? @dkonopka @Comandeer

We used to have figure.image (1,1) to raise the specificity. I propose .ck-content image, which has the same specificity, but clearly defines the purpose of the style.

The icon, OTOH, is a different story. We had svg.ck-icon (1,1) to raise specificity but since this is the UI we can use .ck.ck-icon (2,0), which is stronger and clearer (I did it wrong in the PR — svg is unnecessary).

@Reinmar Reinmar merged commit 9291fb1 into master Mar 29, 2018
@Reinmar Reinmar deleted the t/494 branch March 29, 2018 15:38
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.

3 participants