There is too much going in Bug 947242 to try to switch to new the colors *and* converting to variables in the same patch - It's too hard to keep track of. So let's take 2 separate steps - first keep the existing colors / mappings but convert them to variables here. Then I can switch over and try to match the mockups separately in 947242.

You've already reviewed this mostly in Bug 947242, the only difference is that now there should be no UI changes with the patch applied. I'm just keeping the existing colors in place but extracting them into variables.

This has already been r+ed over on bug 947242, just rebased and removed the changes to markup-view.xhtml (since that requires remapping to the new theme colors).

Comment on attachment 8526147 [details] [diff] [review] just-convert-to-variables-part1.patch Review of attachment 8526147 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/shared/devtools/dark-theme.css @@ +30,5 @@ > + --theme-highlight-purple: #6270b2; > + --theme-highlight-lightorange: #a18650; > + --theme-highlight-orange: #b26b47; > + --theme-highlight-red: #bf5656; > + --theme-highlight-pink: #a673bf; I'm a bit worried that adding the name of the color itself in the variables might lead to some problems in the future (e.g. if pink becomes green on a different theme). I'll let you decide if we want to use 1..n instead for identifying these.

(In reply to Victor Porof [:vporof][:vp] from comment #3) > Comment on attachment 8526147 [details] [diff] [review] > just-convert-to-variables-part1.patch > > Review of attachment 8526147 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/themes/shared/devtools/dark-theme.css > @@ +30,5 @@ > > + --theme-highlight-purple: #6270b2; > > + --theme-highlight-lightorange: #a18650; > > + --theme-highlight-orange: #b26b47; > > + --theme-highlight-red: #bf5656; > > + --theme-highlight-pink: #a673bf; > > I'm a bit worried that adding the name of the color itself in the variables > might lead to some problems in the future (e.g. if pink becomes green on a > different theme). I'll let you decide if we want to use 1..n instead for > identifying these. Hmm, that's a good point. Some colors have definite connotations - like you would want to use theme-highlight-red if there was an error for instance. And it'd be much more convenient when using this in your own file (and outside of the light-theme / dark-theme files) if we kept the name to match the color. However, there is some mismatch when using one of the theme-fg-color-* classes (which may not be consistent across themes). As an author you might say: 1. If I care about the color in particular (red for error, green for valid, etc) or I need to do something special (more than just set the color) then I will add some styles in my own CSS and use the variables 2. If I just want to use some styles that the devtools theme has mapped and I'm only wanting the text color to change, I can use the theme-fg-color-* class. And I think that's generally probably OK. But the case that doesn't cover is: 3. I want to use whatever devtools has decided on, but I want to set something besides the color property The current solution doesn't cover case 3, but that's hopefully rare enough that it won't be a huge problem. Like, 'I want to use the color that the theme thinks corresponds to a tag name, but I want to style something's background with that'. In this case you could probably just use purple or something. There is also no real meaning in fg-color-1 or instructions for when you should use it, so I think for most people authoring panels just sticking to a known color would be fine (unless if you are working with variables / markup / js syntax or something else that we've decided on colors for). And for these we probably should move towards having more specific class names that actually have a meaning like .theme-tag-name, .theme-attribute-name, .theme-js-value, .theme-color-red, .theme-color-blue, etc to cover these cases. I think it might even be nice if we could eventually kill the theme-fg-color-* classes.

https://hg.mozilla.org/integration/fx-team/rev/38d12e852c4b https://hg.mozilla.org/integration/fx-team/rev/d99483864a61

https://hg.mozilla.org/mozilla-central/rev/38d12e852c4b https://hg.mozilla.org/mozilla-central/rev/d99483864a61