Closed Bug 996622 Opened 6 years ago Closed 6 years ago

[rule view] Use new theme colors for marking changed rules

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(3 files, 1 obsolete file)

I'm going to merge the three OS specific ruleview.css files into one, then update that one to use the new colors to show the green left border next to changed rules
Blocks: 947242
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attached image ruleview-changes.png
Not a huge change, just a 1px bigger border and using theme colors from https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors to make the changes a bit more noticable
Attached patch ruleview-changes-pt1.patch (obsolete) — Splinter Review
This patch just pulls the three separate ruleview.css files into one.  There were two differences that were only in OSX (Windows and Linux were the same):

In .ruleview-header there was a margin-top: 4px in Windows/Linux but not OSX.  I've wrapped this in an %ifndef XP_MACOSX to keep the same styling.
 
In OSX the following rule existed but wasn't in the others:

+.ruleview-rule-pseudo-element {
+  padding-left:20px;
+  border-left: solid 10px;
+}

This puts the border next to pseudo elements.  I'm not sure why it isn't in the others - I've added it back in unconditionally, but we could add it only in OSX if it has caused an issue.   I've pushed to try it out on windows/linux: https://tbpl.mozilla.org/?tree=Try&rev=5aabca06a2d2.
Attachment #8406883 - Flags: review?(pbrosset)
This uses the updated theme colors, and adds 1px extra to the border to call more attention to it.  I took the screenshot from attachment 8406876 [details] with both of these patches applied.
Attachment #8406884 - Flags: review?(pbrosset)
Blocks: 896733
Attachment #8406883 - Flags: review?(pbrosset) → review+
Comment on attachment 8406884 [details] [diff] [review]
ruleview-changes-pt2.patch

Review of attachment 8406884 [details] [diff] [review]:
-----------------------------------------------------------------

The screenshot looks great.
Attachment #8406884 - Flags: review?(pbrosset) → review+
Comment on attachment 8406883 [details] [diff] [review]
ruleview-changes-pt1.patch

Review of attachment 8406883 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/devtools/ruleview.css
@@ +21,5 @@
> +  border-bottom-style: solid;
> +  padding: 1px 4px;
> +%ifndef XP_MACOSX
> +  margin-top: 4px;
> +%endif

For some reason having this condition end before the -moz-user-select causes test failures like: https://tbpl.mozilla.org/php/getParsedLog.php?id=37858954&tree=Fx-Team.  I will upload a new patch where this condition happens at the end of the rule
Excited about how spectacularly I was able to break the test by changing only a couple lines of CSS.  Turns out I forgot to add the asterisk in front of the css to enable CSS preprocessing, so the following -moz-user-select property didn't get applied, which caused the test failures.  New push: https://tbpl.mozilla.org/?tree=Try&rev=fe3569221e41
Attachment #8406883 - Attachment is obsolete: true
Attachment #8407043 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/854848b9722d
https://hg.mozilla.org/mozilla-central/rev/ba0fc8bc4aa0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
QA Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.