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

Plugins should not depend on a particular theme – aka support for multiple themes #5304

Closed
Reinmar opened this issue Jan 19, 2017 · 6 comments · Fixed by ckeditor/ckeditor5-ui#345
Assignees
Labels
package:ui status:discussion type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Jan 19, 2017

@oskarwrobel pointed out that the link package requires the lark package now because of this: https://github.com/ckeditor/ckeditor5-link/blob/69cc9fe393698b9689307c209f8d496e6c1f0769/theme/components/linkform.scss#L4

My first thought was – move this spacing helper to ckeditor5-ui because plugins should not depend on a specific theme.

However, the spacing helpers shouldn't be defined by the ckeditor5-ui wireframe theme. So this is more complicated. A plugin's theme can depend on:

  • @ckeditor/ckeditor5-ui/theme/*
  • @ckeditor/ckeditor5-theme/theme/_helpers/*

What is the second package – ckeditor5-theme? It's the theme which the developer chose upon running a bundler. This is the theme which all packages will import (e.g. the classic editor). It won't really exist, so the bundler will resolve its path to an existing theme.

E.g. if you'd configure CKEditorWebpackPlugin with theme: 'xyz', it would resolve all requests to @ckeditor/ckeditor5-theme to @ckeditor/ckeditor5-theme-xyx.

This, as understood by my limited brain, will implement support for multiple themes.

The remaining bit is that every theme must import the same set of _helpers, so we need to standardise them. And I don't think that they should be kept in _helpers but rather helpers.

PS. The tricky thing here will be to how to run this on CI ;> The default theme will need to be lark and we'll need to add it as a dev dep of packages which require any theme files.

@Reinmar
Copy link
Member Author

Reinmar commented Jan 19, 2017

This is an RFC ofc :P

@oleq @oskarwrobel @postJScriptum

@Reinmar
Copy link
Member Author

Reinmar commented Jan 19, 2017

BTW, there's one more, very important, aspect. Helpers MUST NOT introduce any rules. They may introduce mixins and variables, but not final styles. Otherwise, those styles will be included in output CSS multiple times.

Example of a bad helper – _states.scss:

@oleq
Copy link
Member

oleq commented Jan 20, 2017

My limited brain tells me ckeditor5-link/theme/components/linkform.scss should be split into:

  1. ckeditor5-link/theme/ui/components/linkform.scss a wireframe for LinkFormView
  2. ckeditor5-link/theme/lark/components/linkform.scss Lark theming importing above, because Lark is the default CKE5 theme supported by CKSource.

If there comes another theme, the author must import 1. and on top of that style the form according to their needs.

@Reinmar
Copy link
Member Author

Reinmar commented Jan 20, 2017

That's another thing. How a certain package might bring CSS for a certain theme... This part is super scary, but... I think that we should do whatever is possible to make it unnecessary.

This means that we must first try to refactor the theme in such a way that a link will use only the generic parts of a chosen theme, not a specific theme's files.

@Reinmar
Copy link
Member Author

Reinmar commented Mar 22, 2017

Related topic: ckeditor/ckeditor5-engine#872 (comment)

@oleq
Copy link
Member

oleq commented Oct 16, 2017

TL;DR: After some research, I came up with the conclusion that the tool that handles themes in our ecosystem and solves the style duplication issue too is PostCSS. However, the migration from SASS isn't painless.

Solution to the theme duplication problem

The postcss-import plugin, which is maintained by postcss comes with the skipDuplicates option enabled by default.

I simulated multiple editor types scenario and I found out that this importer library prevents duplication that SASS cannot handle:

in SASS the output is:

reset.css imported by ckeditor5-edior-foo
ckeditor5-edior-foo styles
reset.css imported again by ckeditor5-edior-bar
ckeditor5-edior-bar styles

while postcss-importer optimizes it to

reset.css imported by ckeditor5-edior-foo (-bar)
ckeditor5-edior-foo styles
ckeditor5-edior-bar styles

How to implement, load and optimize themes?

The idea is that the ckeditor5-ui package and other packages that also provide UI components (e.g. ckeditor5-link) will define base styles only. At this moment, only ckeditor5-ui follows this rule.

The base styles must be limited and as simple as possible, so any theme can build on top this foundation and achieve the expected result. The structure of ckeditor5-ui will remain almost the same

ckeditor5-ui
    src
        button
            buttonview.js -> import ../theme/button.css
        toolbar
            toolbarview.js  -> import ../theme/toolbar.css
    theme
        button.css (base)
        toolbar.css (base)

but ckeditor5-link will no longer provide an "all–in–one" theme.css file which loads all the styles for the package. A separate CSS file will be imported by each View class instead, to

  • allow easy dependency injection,
  • optimize the size of the build (what isn't used will not be styled),
  • allow per–component theming
ckeditor5-link
    src
        ui
            linkformview.js -> import ../theme/linkformview.css
        link.js
    theme
        linkformview.css (base)

Editor packages will also follow the stylesheet–per-view rule:

ckeditor5-editor-classic
    src
        classiceditoruiview.js -> import ../theme/classiceditoruiview.css
    theme
        classiceditoruiview.css (base)

Then finally, the theme package will provide stylesheets corresponding with each view, supplementing base styles with colors, fonts, borders, spacings, etc.:

ckeditor5-theme-foo
    theme
        ckeditor5-ui
            button.css (theme–specific)
            toolbar.css (theme–specific)
        ckeditor5-link
            linkformview.css (theme–specific)
        ckeditor5-editor-classic
            classiceditoruiview.css (theme–specific)

How to glue the base themes in packages with the actual theme?

Since PostCSS is a JavaScript tool, it's easy to write custom plugins. They can be defined as npm imports and used in postcss.config.js or even directly in webpack.config.js, although the later is not the best idea, really.

I created a simple theme loader which obtains the path to the theme and for each base theme file found:

  1. Looks for the corresponding file in the theme.
  2. If found, it loads it with postcss-import and appends to the base file.
  3. If not, it ignores it.

It basically does the job of the postcss-theme plugin but

  1. without explicit
    @import theme(ckeditor5-ui/button/button.css)
    declaration at the beginning of each base stylesheet.
  2. it is failsafe, so developers don't need to provide a complete set of theme CSS files to successfully complete the building of the editor (major flaw of postcss-theme),
  3. generates styles in the right order. In PostCSS @import statement must be at the beginning of the file and thus the theme files would precede base styles, which is not the best thing considering that in CSS order matters when specificity is being resolved.

The postcss.config.js file using the custom importer is as simple as

'use strict';

const path = require( 'path' );
const postcssImport = require( 'postcss-import' );
const postcssCssnext = require( 'postcss-cssnext' );
const CKThemeImporter = require( './ck-theme-importer' );

module.exports = {
    plugins: [
        postcssImport(),
        CKThemeImporter( {
            themePath: path.resolve( __dirname, 'packages/ckedior5-theme-foo' )
        } ),
        postcssCssnext()
    ]
};

Cons of PostCSS and general migration issues

PostCSS solves our issues, allow easy extension with JavaScript plugins and feels comfortable but there's still major a pitfall of the migration.

By itself, PostCSS offers nothing to make the creation and management of CSS easier. No mixins, variables, function, extends, etc.. Without plugins, PostCSS is a pass-through utility. And unfortunately, the existing plugins fall short compared our experience with SASS.

The most promising plugin I think we should use is CSSNext. It offers the CSS functionality unavailable in most web browsers but slowly becoming standards. It's like Babel but for CSS. The plugin would give us variables

:root {
  --mainColor: red;
}

a {
  color: var(--mainColor);
}

@apply

:root {
  --danger-theme: {
    color: white;
    background-color: red;
  };
}

.danger {
  @apply --danger-theme;
}

nesting, and many other useful utils to deals with colors, units, etc. It does not offer mixins, though, which is a sad news for us because ATM we rely on this feature (e.g. see the button.scss file).

There are other plugins that offer SASS–like mixins, e.g. postcss-mixins but the functionality is limited (no if statements, no loops), and the popularity of the plugin is low. If one day the plugin becomes obsolete and it is already widespread in our ecosystem, it will be a pain to maintain it on our own or remove it without a major uproar in the community.

TBH, I'd rather see fewer plugins even if that would mean our code is longer because the lower maintenance cost will pay off in the long run. CSSNext is a good start IMO because in the future, the features it provides will be supported natively by web browsers and that means a smooth transition at some point.

Can we live without mixins?

What other plugins could we use make our life easier?

Rollup

I checked the integration with Rollup and the simple rollup.config.js works properly:

import postcss from 'rollup-plugin-postcss';
import resolve from 'rollup-plugin-node-resolve';

export default {
    input: './src/ckeditor.js',
    plugins: [
        resolve(),
        postcss( {
            plugins: require( './postcss.config.js' ).plugins
        } )
    ],
    output: {
        file: './build/rollup/ckeditor.js',
        format: 'umd'
    },
};

Theming vs. fast customization

A quick customization using the variables without creating a new theme should still be possible. The postcss-cssnext (CSSNext) plugin offers variable customization directly in the postcss.config.js:

module.exports = {
    plugins: [
        ...
        postcssCssnext( {
            features: {
                customProperties: {
                    variables: {
                        ck-color-fg: "red",
                        ck-color-bg: "green"
                    }
                }
            }
        } )
    ]
};

It's a similar feature to options.data in the sass-loader.

@oleq oleq self-assigned this Nov 22, 2017
oleq referenced this issue Nov 30, 2017
Docs: Migrated the snippet adapter to PostCSS (see ckeditor/ckeditor5-ui#144).
oleq referenced this issue in ckeditor/ckeditor5-theme-lark Nov 30, 2017
Other: Migrated the theme from SASS to PostCSS (see ckeditor/ckeditor5-ui#144).

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](https://docs.ckeditor.com/ckeditor5/latest/framework/guides/ui/theme-customization.html) guide to learn more about migration to PostCSS.
oleq referenced this issue in ckeditor/ckeditor5-basic-styles Nov 30, 2017
Other: Migrated package styles to PostCSS. Moved the visual styles to ckeditor5-theme-lark (see ckeditor/ckeditor5-ui#144).
oleq referenced this issue in ckeditor/ckeditor5-block-quote Nov 30, 2017
Other: Migrated package styles to PostCSS. Moved the visual styles to ckeditor5-theme-lark (see ckeditor/ckeditor5-ui#144).
oleq referenced this issue in ckeditor/ckeditor5-build-balloon Nov 30, 2017
Other: Changed the webpack configuration so the styles are processed using PostCSS instead of SASS (see ckeditor/ckeditor5-ui#144).
oleq referenced this issue in ckeditor/ckeditor5-build-classic Nov 30, 2017
Other: Changed the webpack configuration so the styles are processed using PostCSS instead of SASS (see ckeditor/ckeditor5-ui#144).
oleq referenced this issue in ckeditor/ckeditor5-build-inline Nov 30, 2017
Other: Changed the webpack configuration so the styles are processed using PostCSS instead of SASS (see ckeditor/ckeditor5-ui#144).
oleq referenced this issue in ckeditor/ckeditor5-editor-balloon Nov 30, 2017
Other: Migrated the editor styles to PostCSS (see ckeditor/ckeditor5-ui#144).
oleq referenced this issue in ckeditor/ckeditor5-editor-inline Nov 30, 2017
Other: Migrated the editor styles to PostCSS (see ckeditor/ckeditor5-ui#144).
oleq referenced this issue in ckeditor/ckeditor5-editor-classic Nov 30, 2017
Other: Migrated the editor styles to PostCSS. Moved visual styles to ckeditor5-theme-lark (see ckeditor/ckeditor5-ui#144).
oleq referenced this issue in ckeditor/ckeditor5-engine Nov 30, 2017
Other: Migrated package styles to PostCSS. Moved visual styles to ckeditor5-theme-lark (see ckeditor/ckeditor5-ui#144).
oleq referenced this issue in ckeditor/ckeditor5-heading Nov 30, 2017
Other: Migrated package styles to PostCSS. Moved visual styles to ckeditor5-theme-lark (see ckeditor/ckeditor5-ui#144).
oleq referenced this issue in ckeditor/ckeditor5-link Nov 30, 2017
Other: Migrated package styles to PostCSS. Moved visual styles to ckeditor5-theme-lark (see ckeditor/ckeditor5-ui#144).
oleq referenced this issue in ckeditor/ckeditor5-image Nov 30, 2017
Other: Migrated package styles to PostCSS. Moved visual styles to ckeditor5-theme-lark (see ckeditor/ckeditor5-ui#144).
oleq referenced this issue in ckeditor/ckeditor5-upload Nov 30, 2017
Other: Migrated package styles to PostCSS. Moved visual styles to ckeditor5-theme-lark (see ckeditor/ckeditor5-ui#144).
oleq referenced this issue in ckeditor/ckeditor5-widget Nov 30, 2017
Other: Migrated package styles to PostCSS. Moved visual styles to ckeditor5-theme-lark (see ckeditor/ckeditor5-ui#144).
oleq referenced this issue in ckeditor/ckeditor5-ui 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.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-ui Oct 9, 2019
@mlewand mlewand added this to the iteration 14 milestone Oct 9, 2019
@mlewand mlewand added status:discussion type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:ui labels Oct 9, 2019
This was referenced Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:ui status:discussion type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants