Standardize Theming - Githubissues

matthias-vogt commented 8 years ago

The theme CSS files are currently very inconsistent and have a lots of redundancy among each other.

Moreover, by some themes (like solarized or ambiance) changing the layout (e.g. by adding padding to the editor), its hard to create a theme agnostic site where changing themes doesn't break the site's layout.

In principle, should themes only be able to change the editor's color scheme or should they also be able to introduce layout changing CSS? (pls no)

I think the best solution would be to put the themes in a .json and auto generate the theme files using npm scripts. Alternatively, one could use Sass and make _partials for each theme where variables for colors would be defined and if themes should be able to introduce custom CSS, that could be in those files too.

This could also help prevent future cleanups like https://github.com/codemirror/CodeMirror/pull/3427

Would you accept pull requests for this?

vincentwoo commented 8 years ago

I personally would be happy to see something like this in place, but would also want to see a more concrete proposal for how the system would work in practice. For instance, what would the JSON files look like? Are you going to try to whitelist css properties?

matthias-vogt commented 8 years ago

With the JSON approach, one could either…

make a .json for every theme 1.1 have the theme name in the .json 1.2 have the theme name in the file name
make one .json for all themes

I'd suggest 1.2 for modularity and little redundancy. For instance the default color scheme…

.cm-s-default .cm-keyword {color: #708;}
.cm-s-default .cm-atom {color: #219;}
.cm-s-default .cm-number {color: #164;}
.cm-s-default .cm-def {color: #00f;}
…

…could become

themes/default.json (using 1.2)

{
    "keyword": "#708",
    "atom": "#219",
    "number": "#164",
    "def": "#00f",
    …
}

themes/themes.json (using 2)

{
    "default": {
        "keyword": "#708",
        "atom": "#219",
        "number": "#164",
        "def": "#00f",
        …
    },
    "other theme": {
        "keyword": "red",
        "atom": "green",
        "number": "rgb(0,0,0)",
        "def": "orange",
        …
    }
    …
}

If a property isn't defined, it should not fall back to the default theme as the default theme is always included in the codemirror.css file and will provide a fallback for missing properties.

Since the existing themes have very individual styling, we'll have to draw a line between what a theme should be able to customize and what not and make an API for allowed JSON properties accordingly.

marijnh commented 8 years ago

I have very little interest in adding build steps and other complexity to the themes, especially if the intended effect is to reduce their power. If you want to have a set of color-only themes, maybe set it up as a separate project.

matthias-vogt commented 8 years ago

I respect your opinion and I too want to reduce complexity and think that a build step would actually be a reasonable investment to do so. The theme files really are a mess right now, they lack of convention and have very unpredictable effects. If you prefer individuality over convention and predictability for themes, maybe we could separate the color scheming and layout components from each other? And make layout changing default and optional? That way, we could have the best of both worlds, themes that can change the editor however they want to by default and, optionally, the dev being able to offer any color scheme to the user without having to fear of his page layout breaking.

If we had a sass partial, for example, we could do something like this:

name.scss

@import 'interpreter';

@include color-scheme((
    keyword: #708,
    atom: #219,
    number: rgb(200, 141, 50),
    def: #00f
    //…
));

@include layout("name") {
  .CodeMirror-linenumber {
    padding: 0 .4em;
  }
}

_ìnterpreter.scss

$use-layout: true !default;

@mixin color-scheme($scheme) {
  /* Color Scheme */
  @each $prop, $color in $scheme {
    .cm-#{$prop} { color: $color; }
  }
}

@mixin layout($theme-name) {
  @if $use-layout {
    /* Layout */
    .cm-s-#{$theme-name} {
      @content;
    }
  }
}

You can see a working example here: http://www.sassmeister.com/gist/f28dc0b8aae5823f6302

Edit: I just realized that, since a theme will probably want to change background colors and colors not belonging to syntax highlighting, the demo above isn't really a good approach. I've extended it to allow using background colors and added some custom tokens for changing colors beyond syntax highlighting. http://www.sassmeister.com/gist/ecb4964e10160914f2d5

vincentwoo commented 8 years ago

I think (since you'd have to do this work anyway) you might be better off just hand-auditing all the themes, then producing like a "theme-master" template that also talks about which properties are "safe" to change. Like @marijnh said, it might be overkill to introduce this as a build-time thing, and some themes conceivably could use the extra freedom (I recall one theme using an image background). I agree that a good, safe, guideline is a good thing to have, though.

matthias-vogt commented 8 years ago

That wouldn't solve the problem of making layout-changing CSS optional, though. This is kinda the perfect fit for a build step as it's the only way to effectively build a layer of abstraction on top of theming (until CSS variables have support :pray:). I feel like building this layer is the most sustainable and empowering solution because it…

makes it very easier to port color schemes from other editors to Codemirror themes
reduces redundancy among theme definitions
enforces a style guide
- makes themes more predictable
- will prevent future code cleanups
allows individuality among themes
still gives developers the option of standardizing them where necessary (use only color schemes)
- allows theme agnostic development

It's just a nicer system, easier to work with and from my point of view completely justifies the effort to introduce a build step. I can actually see this being a better API for theme creators than the current one—even though it's not pure CSS.

As a side effect, the Sass technique would also give developers the option of using the theme's color variables independently of their theme. So, for example in the rest of their code to be able to customize their page toward the Codemirror theme. If you make a site where the Codemirror editor is centric, this is a super cool benefit.

codemirror / codemirror5

Standardize Theming #3902