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

Styles collision and duplication when using different editors #420

Closed
oleq opened this issue Mar 31, 2017 · 5 comments · Fixed by ckeditor/ckeditor5-ui#345
Closed

Styles collision and duplication when using different editors #420

oleq opened this issue Mar 31, 2017 · 5 comments · Fixed by ckeditor/ckeditor5-ui#345
Labels
type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@oleq
Copy link
Member

oleq commented Mar 31, 2017

To experience the problem, create one ClassicEditor and one InlineEditor instance:

image

Source of the problem:

// The first one loads classic/theme/theme.scss...
// which loads Larks with resets.
import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classic';

// The first one loads inline/theme/theme.scss...
// which again loads Larks with resets.
import InlineEditor from '@ckeditor/ckeditor5-editor-inline/src/inline';
...

so the final order of styles is

  1. Reset.
  2. Classic styles
  3. Reset again, which overrides classic styles, because it's after it.
  4. Inline styles, which work just fine because they follow both resets.

So the problem we need to solve is the duplication. It's not only about reset. Tons of styles are duplicated in such situation. How do we deal with it?

/cc @Reinmar

@oleq oleq added candidate:1.0.0 type:bug This issue reports a buggy (incorrect) behavior. labels Mar 31, 2017
@Reinmar
Copy link
Member

Reinmar commented Mar 31, 2017

So the problem we need to solve is the duplication. It's not only about reset. Tons of styles are duplicated in such situation. How do we deal with it?

I thought about this recently and just waited for the first bug report. No idea... really. I hate CSS management. And now I also hate that SASS doesn't deduplicate the imports. Because the problem starts there. So when looking for the solution look for "SASS deduplicate imports" or "import once".

👺

@oleq
Copy link
Member Author

oleq commented Mar 31, 2017

Yeah, we're not the first to discover the problem webpack-contrib/sass-loader#145 :D

@Reinmar
Copy link
Member

Reinmar commented May 23, 2017

This is so sad 😭 😢 😞 😿 . We're having now a problem in documentation because I want to include examples of two builds in one guide. There's actually no solution in CSS world for that. We'd need to be impossibly careful to write such CSS rules that don't conflict when included twice because in such case even import_once wouldn't help (builds are generated separately). So frustrating ;/

Anyway, no collisions between two separately generated builds are impossible to achieve. So the minimum is that we're able to create one entry file which imports themes of two creators and that these creators don't conflict.

With Sass, this seems to be impossible. One of the creators' themes looks like this:

@import '~@ckeditor/ckeditor5-theme-lark/theme/theme.scss';

@include ck-editor.ck-creator-foo {
    ... creator specific rules
}

And the other looks the same but contains different "creator specific rules".

Now, I'd like to be able to create an app in which I'd import both of these files. What will Sass do? It will include theme-lark twice 😡 leading to issues like @oleq described. There's apparently no solution (suitable in general cases like ours) to that.

I'm honestly considering switching to Less. This means a lot of work at this stage but what we're facing here is a complete blocker for which I can't think of any sane workaround. We chose Sass because it seemed more mature and more actively maintained, but sass/sass#139 is closed for 2 years and there's still no version 4.0 which would release this feature.

@Comandeer
Copy link
Member

Current implementation of Sass is going to be abandoned. So “mature” project that is going to be rewritten in Dart…

TBH I’d investigate Post CSS, it has even plugins for Sass compatible syntax.

CSSM has only one feature: imports. So it’s not a suitable option to replace preprocessor (not on its own). LESS is… well, it is and that’s all. Stylus is nice, but it has no documentation, no sensible error codes… The only reasonable choice is PostCSS

@m-kr
Copy link

m-kr commented Jun 4, 2017

I totally agree with @Comandeer.

LESS is very niche now the same as GRUNT, TBH I can't remember new, modern app which is using LESS, even Bootstrap 4 will be written in SASS. Using LESS may discourage the community from involving in development.

However, it doesn't mean that SASS is the best preprocessor, it's only the most popular. I've done research and PostCSS has great import plugin which has skipDuplicates option. And many others which can improve styles scoping. I'm not sure if those plugins will resolve all described problems but definitively can eliminate most of them.

Regarding migration from SASS, PostCSS has cssnext package with many plugins which make syntax similar or even the same as SASS, so IMHO migration might be not so painful.

oleq added a commit to ckeditor/ckeditor5-ui that referenced this issue Nov 30, 2017
Other: Migrated the package styles from SASS to PostCSS to bring theme support and avoid duplicates in some editor builds. Closes #144. Closes ckeditor/ckeditor5#420.

BREAKING CHANGE: The styles are no longer developed in SASS which means the `.scss` files became unavailable. Please refer to the [Theme Customization](https://docs.ckeditor.com/ckeditor5/latest/framework/guides/ui/theme-customization.html) guide to learn more about migration to PostCSS.
@Reinmar Reinmar added this to the iteration 14 milestone Dec 5, 2017
@Reinmar Reinmar mentioned this issue Oct 9, 2019
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants