Closed Bug 1102369 Opened 10 years ago Closed 10 years ago

Convert current devtools colors to css variables

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(2 files)

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.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8526147 - Flags: review?(vporof)
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).
Attachment #8526158 - Flags: review+
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.
Attachment #8526147 - Flags: review?(vporof) → review+
(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.
Keywords: checkin-needed
Blocks: 1102464
Blocks: 1102084
https://hg.mozilla.org/mozilla-central/rev/38d12e852c4b
https://hg.mozilla.org/mozilla-central/rev/d99483864a61
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Depends on: 1104856
Blocks: 1106124
Depends on: 1107972
No longer blocks: 1106124
Depends on: 1106124
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.