simurai / duotone-light-syntax

DuoTone light - A syntax theme for Atom
MIT License
58 stars 5 forks source link

Angular JS colors in html don't follow the pattern #4

Closed smlombardi closed 8 years ago

smlombardi commented 8 years ago

On "HTML(Angular)" documents, more scopes are exposed in html. See below. I admit Atom is not consistent in this area, but things like ng-repeat are colored differently than regular html attributes. This is ok, maybe even a bonus, except in your theme the color is weak compared to a regular attribute.

What's worse is that the attribute string doesn't match other attributes strings; they are black, or "regular text" color instead of blue. Now, you might elect to color them differently than other attribute values (some, but not all, have new scopes) which would be great, but if not blue would at least match.

You'll also notice the placeholder structure {{item.label}} is fully colored and doesn't match the rest of your theme, which is otherwise excellent.

2015-12-14_13-30-02

You can download the html file in question here: http://cl.ly/1Q3t2O0n3k0H/header.html

simurai commented 8 years ago

@smlombardi That link doesn't seem to render correctly. I just typed this part to test:

<ul class="list">
  <li ng-repeat="item in item.children">
    <a href="" ui-sref="{{item.value}}" >{{item.label}}</a>
  </li>
</ul>

What "Angular" package are you using? Maybe https://atom.io/packages/angularjs?

Haa.. that's kinda interesting.. it seems that package overrides ng-repeat with @text-color-warning from the ui-variables https://github.com/angular-ui/AngularJS-Atom/blob/master/styles/angularjs.less#L11. With the One light UI theme it looks okayish, but depends on the UI theme.

screen shot 2015-12-20 at 6 02 21 pm

I guess as a quick fix, you could override the colors in your styles.less? Long term I'm not sure. Typically language packages only define the scope, but don't style anything. In this case I can see the reasoning to highlight these custom attributes differently than normal HTML attributes.

simurai commented 8 years ago

The angularjs package has the following options.

  1. No special styling. ng-repeat will just show like the rest of the attributes.
  2. Call ng-repeat something different than attribute-name so that themes will style it differently. Maybe support type or so.
  3. Use a color variable from syntax-variables. So it matches the rest of the syntax theme. Maybe one of these: https://github.com/atom/atom/blob/master/static/variables/syntax-variables.less#L32-L44 but not all themes added them yet.. this theme is guilty too.

/cc @outsideris

smlombardi commented 8 years ago

Thanks

outsideris commented 8 years ago

@simurai Thank you for the mention. I was trying to follow atom guide, but I haven't made themes for Atom ever. Do you have any suggestions to be compatible with themes?

simurai commented 8 years ago

@outsideris I guess it depends a little how important it is that the Angular directives have a different color than normal attributes:

I can make a PR to angularjs once we settle on one of the above options.

izelnakri commented 8 years ago

@outsideris , I think @simurai is expecting an answer to those 3 points before making any pull request.

outsideris commented 8 years ago

@izelnakri Sorry for late answering. I was busy by other work. I have to check it before I answer to @simurai . I expect I can do it in a couple of weeks.

simurai commented 8 years ago

@outsideris No rush. I kinda tend towards the 2/medium important option. But it might be hard to find something matching that is part of the TextMate naming conventions.

outsideris commented 8 years ago

@simurai Sorry for super late. The overriding theme in a plugin is not good, I think. It is over plugin's scope. I didn't know how to make themes in Atom and Textmate when I wrote AngularJS-Atom plugin..

I agree with you that 2/medium important is the best option. I tried to find the class name which many themes support. It is much hard as you said. Many AngularJS users want highlight angular particular attribute or expression, and I can't ignore this request.

So, I choose support.other.attribute-name.html.angular for attribute-name rather than entity.other.attribute-name.html.angular and remove meta.tag.template.angular for attribute value, not angular express like {{...}}.

I tested it with some themes including duotone. Some themes are better than before, and some themes are not much better than before. I think this not-aggressive approach is better than one I used.

Below screenshot is before I change classes.

atom-duol-01

and below is after I change them.

atom-duol-02

I want to hear how do you think about support.other.attribute-name.html.angular because I am not theme author. I followed Textmate document.

support — things provided by a framework or library should be below support.

simurai commented 8 years ago

From TM's convention:

support — things provided by a framework or library should be below support.

So I think support.other.attribute-name.html.angular is a good option. Most themes probably don't style the .attribute-name.html.angular part, but the chance for support and support.other should be higher, so maybe ok. :+1:

outsideris commented 8 years ago

@simurai Thank you for feedback. :100: