Closed Bug 683906 Opened 13 years ago Closed 13 years ago

Make the Highlighter Toolbar look like shorlander's design

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 10

People

(Reporter: paul, Assigned: paul)

References

Details

(Whiteboard: [minotaur])

Attachments

(2 files, 3 obsolete files)

We may want to use OSX design (bug 676253) or Windows design (Bug 676255).
Attached patch patch v1 WIP (obsolete) — Splinter Review
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attached image patch v1 - screenshot - ubuntu 10.04 (obsolete) —
This is Ubuntu 10.04 LTS, clearlooks theme, gnome 2.30, gtk2.

The various Inspect button states:

http://img.i7m.de/show/nxwuk.png

Third state, mouse over, does not look too good for now.
Thank you Mihai.
Whiteboard: [minotaur]
Blocks: 672006
Attached patch patch v1 (obsolete) — Splinter Review
Based on Mac design (bug 676253).
Attachment #557581 - Attachment is obsolete: true
Attachment #557619 - Attachment is obsolete: true
Attachment #559097 - Flags: review?(dao)
Comment on attachment 559097 [details] [diff] [review]
patch v1

>+#inspector-toolbar {
>+  -moz-appearance: none;
>+  height: 32px;

Why are you setting a height here rather than letting the toolbar be sized automatically based on its contents?

>+#inspector-inspect-toolbutton,
>+#inspector-tools > toolbarbutton {
>+  -moz-appearance: none;
>+  width: 78px;

I think you want min-width here.

>+  margin: 3px 5px !important;

Why !important?

>+#inspector-inspect-toolbutton[checked],
>+#inspector-tools > toolbarbutton[checked] {
>+  color: hsl(208,100%,60%) !important;
>+  border-color: hsla(210,8%,5%,.6) !important;
>+  background: -moz-linear-gradient(hsla(220,6%,10%,.6), hsla(210,11%,18%,.45) 75%, hsla(210,11%,30%,.4));
>+  box-shadow: 0 1px 3px hsla(210,8%,5%,.25) inset, 0 1px 3px hsla(210,8%,5%,.25) inset, 0 1px 0 hsla(210,16%,76%,.15);
>+}
>+
>+#inspector-inspect-toolbutton[checked]:hover,
>+#inspector-tools > toolbarbutton[checked]:hover {
>+  background-color: transparent!important;
>+}
>+
>+#inspector-inspect-toolbutton[checked]:hover:active,
>+#inspector-tools > toolbarbutton[checked]:hover:active {
>+  background-color: hsla(210,8%,5%,.2)!important;
>+}

Always add a space before !important.
Which of the above !important flags are really needed?
Attachment #559097 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] from comment #5)
> Comment on attachment 559097 [details] [diff] [review]
> patch v1
> 
> >+#inspector-toolbar {
> >+  -moz-appearance: none;
> >+  height: 32px;
> 
> Why are you setting a height here rather than letting the toolbar be sized
> automatically based on its contents?

I'll fix that (for Mac and Windows too).

> 
> >+#inspector-inspect-toolbutton,
> >+#inspector-tools > toolbarbutton {
> >+  -moz-appearance: none;
> >+  width: 78px;
> 
> I think you want min-width here.

Right.

> >+  margin: 3px 5px !important;
> 
> Why !important?

Because we find this in toolbarbutton.css:
.toolbarbutton-text {
  margin: 0 !important;
  text-align: center;
}

> >+#inspector-inspect-toolbutton[checked],
> >+#inspector-tools > toolbarbutton[checked] {
> >+  color: hsl(208,100%,60%) !important;
> >+  border-color: hsla(210,8%,5%,.6) !important;
> >+  background: -moz-linear-gradient(hsla(220,6%,10%,.6), hsla(210,11%,18%,.45) 75%, hsla(210,11%,30%,.4));
> >+  box-shadow: 0 1px 3px hsla(210,8%,5%,.25) inset, 0 1px 3px hsla(210,8%,5%,.25) inset, 0 1px 0 hsla(210,16%,76%,.15);
> >+}
> >+
> >+#inspector-inspect-toolbutton[checked]:hover,
> >+#inspector-tools > toolbarbutton[checked]:hover {
> >+  background-color: transparent!important;
> >+}
> >+
> >+#inspector-inspect-toolbutton[checked]:hover:active,
> >+#inspector-tools > toolbarbutton[checked]:hover:active {
> >+  background-color: hsla(210,8%,5%,.2)!important;
> >+}
> 
> Always add a space before !important.

Ok.

> Which of the above !important flags are really needed?

All of them.
OS: Linux → Mac OS X
OS: Mac OS X → Linux
Merging bug 676255 with this bug. Coming patch will include CSS for Linux, Windows and Mac (pinstripe code updated for consistency with Linux and Windows).
OS: Linux → All
Summary: Make the Highlighter Toolbar look like shorlander's design on Linux → Make the Highlighter Toolbar look like shorlander's design
Attached patch patch v2Splinter Review
In this new patch, I include code for Linux, Windows and Mac. See comment 7.
Attachment #559097 - Attachment is obsolete: true
Attachment #563097 - Flags: review?(dao)
Blocks: 691712
Dão, do you have some time to look at this patch?
Blocks: 663830
review ping!
Comment on attachment 563097 [details] [diff] [review]
patch v2

>--- a/browser/themes/winstripe/browser/browser.css
>+++ b/browser/themes/winstripe/browser/browser.css

>+#inspector-inspect-toolbutton > .toolbarbutton-icon,
>+#inspector-tools > toolbarbutton  > .toolbarbutton-icon {
>+  margin: 0px;

nit: remove "px"
Attachment #563097 - Flags: review?(dao) → review+
Attached patch patch v2.1Splinter Review
Thanks for the r+ Dao
Whiteboard: [minotaur] → [minotaur][land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/359389696002
Whiteboard: [minotaur][land-in-fx-team] → [minotaur][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/359389696002
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Hardware: x86 → All
Resolution: --- → FIXED
Whiteboard: [minotaur][fixed-in-fx-team] → [minotaur]
Target Milestone: --- → Firefox 10
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: