atom / one-dark-syntax

Atom One dark syntax theme
MIT License
443 stars 236 forks source link

Add pug (jade) support #67

Closed webdif closed 8 years ago

webdif commented 8 years ago

Before / after:

before-after

lee-dohm commented 8 years ago

Would you mind creating the same change for https://github.com/atom/one-light-syntax?

webdif commented 8 years ago

Of course. The PR is here: https://github.com/atom/one-light-syntax/pull/17

lee-dohm commented 8 years ago

@simurai what do you think?

simurai commented 8 years ago

The styles seem fine. 👍

But not sure where we wanna draw the line what language should be supported in the default theme. Should it only be languages that come bundled by default with Atom? I guess every selector that gets added comes with a tiny tiny performance cost, even though you might never need those styles. Or is that so small that we shouldn't worry about it?

/cc @atom/languages @atom/feedback

thomasjo commented 8 years ago

I'm personally leaning towards only adding support for languages bundled with Atom. At the same time, I realize that might not provide an optimal end-user experience...

webdif commented 8 years ago

Is it possible to add language support to a theme, via a separated package? That's what I searched for, before ending up with this PR.

lee-dohm commented 8 years ago

@webdif Yes, it is. For example, here is some code from my styles.less that keys off of the theme name:

.theme-one-light-ui, .theme-one-dark-ui {
  .status-bar { font-size: 15px; }
  .tab-bar { font-size: 15px; }
  .tree-view { font-size: 15px; }
}

Atom adds the names of the themes as classes on atom-workspace so that you can do specifically this kind of customization.

On the flip side, a separate package isn't going to have access as easily to the color information you're using in this PR.

simurai commented 8 years ago

In this case, you could hard code the colors in your styles.less file:

.theme-one-dark-syntax atom-text-editor::shadow {
  .source.pug,
  .source.jade {
    .meta {
      &.tag.attribute {
        &.id {
          color: hsl(207, 82%, 66%);
        }
        &.class {
          color: hsl( 29, 54%, 61%);
        }
      }
      &.delimiter {
        color: hsl(  5, 48%, 51%);
      }
    }
    .constant.character.entity {
      color: hsl( 39, 67%, 69%);
    }
  }
}

Of course, the colors could change, unlikely anytime soon, but still. So another option is to try to use the official variables. Other themes might support them too and if not, there is a fallback from core:

@import "syntax-variables";

atom-text-editor::shadow {
  .source.pug,
  .source.jade {
    .meta {
      &.tag.attribute {
        &.id {
          color: @syntax-color-method; // @hue-2
        }
        &.class {
          color: @syntax-color-attribute; // @hue-6
        }
      }
      &.delimiter {
        color: @syntax-color-variable; // @hue-5-2
      }
    }
    .constant.character.entity {
      color: @syntax-color-class; // @hue-6-2
    }
  }
}

But it might not always be easy to map the right variables.

simurai commented 8 years ago

Options I can think of:

  1. Add support in styles.less. See above.
  2. Create a fork, maybe under https://github.com/atom-community/, like one-dark-plus-syntax and accept any language. Downside: Yet another theme to maintain. And it doesn't really solve the "I just need the styles for the languages I use and not everyone else's".
  3. Create a package like language-pug-themes. Then do same as above for styles.less. Downside: Then the question is, which themes should be supported. And with hardcoded colors it's probably too much work to maintain. With using the official variables it's hard to predict how themes style the rest.
  4. The language packages should add the styling. Especially in this case where there are two 1 2 equal popular language packages and after the name change from Jade -> Pug, maybe language-pug might become popular too. Then each language could fine tune the syntax highlighting on their own. Currently they just define the scopes and hope themes use it in the right way and like in this case, add support for it. But that would have to wait for 2.0 so that all themes support the same color variables that language packages can use.
simurai commented 8 years ago

For now I would suggest doing option 1 (styles.less) for non-bundled languages. And a wontfix for this PR.

Then when Atom 2.0 comes around, try to do option 4. So that when you install a language package, syntax highlighting is supported without having to hunt for the right theme or beg your favorite theme to add support.

@webdif Are you ok customizing it in your styles.less for now?

webdif commented 8 years ago

I'm OK with this :+1: