If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Convert current devtools colors to css variables

RESOLVED FIXED in Firefox 36

Status

()

Firefox
Developer Tools
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8526147 [details] [diff] [review]
just-convert-to-variables-part1.patch

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)
(Assignee)

Comment 2

3 years ago
Created attachment 8526158 [details] [diff] [review]
just-convert-to-variables-2.patch

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+
(Assignee)

Comment 4

3 years ago
(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.
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 5

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/38d12e852c4b
https://hg.mozilla.org/integration/fx-team/rev/d99483864a61
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
(Assignee)

Updated

3 years ago
Blocks: 1102464
(Assignee)

Updated

3 years ago
Blocks: 1102084
https://hg.mozilla.org/mozilla-central/rev/38d12e852c4b
https://hg.mozilla.org/mozilla-central/rev/d99483864a61
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
(Assignee)

Updated

3 years ago
Depends on: 1104856

Updated

3 years ago
Blocks: 1106124
(Assignee)

Updated

3 years ago
Depends on: 1107972
(Assignee)

Updated

3 years ago
No longer blocks: 1106124
Depends on: 1106124
You need to log in before you can comment on or make changes to this bug.