Closed Bug 1260523 Opened 8 years ago Closed 8 years ago

Use the new thin icon style from bug 1225184 on toolbar icons

Categories

(DevTools :: General, defect, P2)

defect

Tracking

(firefox50 verified)

VERIFIED FIXED
Firefox 50
Tracking Status
firefox50 --- verified

People

(Reporter: ntim, Assigned: hholmes)

References

Details

(Whiteboard: [btpp-fix-later])

Attachments

(7 files, 8 obsolete files)

546 bytes, image/svg+xml
Details
635.16 KB, image/png
Details
65.61 KB, image/png
Details
74.27 KB, image/png
Details
89.91 KB, image/png
Details
75.58 KB, image/png
Details
109.40 KB, patch
hholmes
: review+
hholmes
: ui-review+
Details | Diff | Splinter Review
      No description provided.
Attached image clear.svg
Priority: -- → P2
Whiteboard: [btpp-fix-later]
Attached patch WIP (obsolete) — Splinter Review
Attached patch WIP 2 (obsolete) — Splinter Review
Attachment #8737547 - Attachment is obsolete: true
Attached patch WIP 3 (obsolete) — Splinter Review
Attachment #8737594 - Attachment is obsolete: true
Attached patch WIP 3.1 (obsolete) — Splinter Review
Attachment #8744440 - Attachment is obsolete: true
Assignee: nobody → hholmes
Blocks: 1268591
Attached patch thin-icons.patch (obsolete) — Splinter Review
Hey Tim,

Tried to pick up where you left off. I pixelsnapped some additional toolbars in addition to cleaning up the debugger icons (step-over, step-in, etc.), the command-no-autohide button (bug 1260560), and a few other odds and ends.
Attachment #8769738 - Flags: review?(ntim.bugs)
Attached patch thin-icons.patch (obsolete) — Splinter Review
Attachment #8769738 - Attachment is obsolete: true
Attachment #8769738 - Flags: review?(ntim.bugs)
Attachment #8769748 - Flags: review?(ntim.bugs)
The new icons are a definite improvement. I really the perf icon :)

I'm seeing some sizing inconsistencies as highlighted in the screenshot, and some shape inconsistencies as well (the shape of both breakpoint icons is different, as well as the { } icon).
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #8)
> Created attachment 8769766 [details]
> critique helen's icons-1.png
> 
> The new icons are a definite improvement. I really the perf icon :)
I really like*
Attached image critique-2.png
Attached image feedback-3.png
Comment on attachment 8769748 [details] [diff] [review]
thin-icons.patch

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

The code changes look good so far.

::: devtools/client/themes/images/itemToggle.svg
@@ +1,4 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg width="16" height="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg" fill="whitesmoke">

You might want to put this icon in sync with debugger-blackbox.svg, since it's supposed to be the same eye icon.

::: devtools/client/themes/images/power.svg
@@ +1,5 @@
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>
> +<svg width="16px" height="16px" viewBox="0 0 16 16" fill="whitesmoke" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:sketch="http://www.bohemiancoding.com/sketch/ns">
> +            <path d="M8.5,14 C5.46243388,14 3,11.5375661 3,8.5 C3,5.46243388 5.46243388,3 8.5,3 C11.5375661,3 14,5.46243388 14,8.5 C14,11.5375661 11.5375661,14 8.5,14 L8.5,14 Z M8.5,13 C10.9852814,13 13,10.9852814 13,8.5 C13,6.01471863 10.9852814,4 8.5,4 C6.01471863,4 4,6.01471863 4,8.5 C4,10.9852814 6.01471863,13 8.5,13 L8.5,13 Z M10.6892468,4.56750356 L11.3722285,3.80863499 L11.3722285,3.80863499 C10.5362548,3.29572747 9.55266283,3 8.5,3 C7.44733717,3 6.46374519,3.29572747 5.62777149,3.80863499 L6.3107532,4.56750356 C6.95873288,4.20599601 7.70530997,4 8.5,4 C9.29469003,4 10.0412671,4.20599601 10.6892468,4.56750356 L10.6892468,4.56750356 L10.6892468,4.56750356 Z" id="Triangle" sketch:type="MSShapeGroup"></path>
> +            <path d="M8,8 L9,8 L9,2 L8,2 L8,8 Z" id="Shape" sketch:type="MSShapeGroup"></path>
> + </svg>
\ No newline at end of file

This SVG needs cleanup

::: devtools/client/themes/images/tool-profiler.svg
@@ +1,5 @@
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg width="16" height="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg" fill="whitesmoke">
> +  <g fill-rule="evenodd">

the <g> tag can be removed, and fill-rule="evenodd" can be set on <svg>

::: devtools/client/themes/images/tool-scratchpad.svg
@@ +1,4 @@
> +<svg width="16" height="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg">
> +  <title>
> +    tool-scratchpad-expanded
> +  </title>

The title can be removed

@@ +1,5 @@
> +<svg width="16" height="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg">
> +  <title>
> +    tool-scratchpad-expanded
> +  </title>
> +  <g fill="whitesmoke" fill-rule="evenodd">

Same comment about the g tag.
Attachment #8769748 - Flags: review?(ntim.bugs) → review+
Attached patch thin-icons.patch (obsolete) — Splinter Review
I know you marked the last patch as r+, but would you mind taking a second look just to be safe? I ended up updating a few more icons, couldn't help myself.

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #8)
> The new icons are a definite improvement. I really the perf icon :)
That icon is all Bryan Bell, can't take credit for it :D

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #8)
> Created attachment 8769766 [details]
> critique helen's icons-1.png
> I'm seeing some sizing inconsistencies as highlighted in the screenshot, and
> some shape inconsistencies as well (the shape of both breakpoint icons is
> different, as well as the { } icon).

I updated the {} icon to be the same. The debugger icon and the toggle breakpoints icon is purposefully different, however.

Updated the shader editor icon, it was a little blurry.

Made the commandnoautohide icon a little larger, but wasn't sure what to do about webaudio and scratchpad—on scratchpad the lines aren't centered if I give it one pixel of height, and when I give it two pixels of height it looks kinda large. Since both it and webaudio aren't /super/ used, I think it's okay to leave them.

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #10)
> Created attachment 8769770 [details]
> critique-2.png
I think the stroke seeming large on the filter icon is visual trickery—I bumped down the size of the "liquid" inside the filter and I think it helps, let me know what you think.

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #11)
> Created attachment 8769772 [details]
> feedback-3.png
Updated!
Attachment #8769748 - Attachment is obsolete: true
Attachment #8769853 - Flags: review?(ntim.bugs)
Attachment #8744441 - Attachment is obsolete: true
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #13)
> Created attachment 8769853 [details] [diff] [review]
> thin-icons.patch
> 
> I know you marked the last patch as r+, but would you mind taking a second
> look just to be safe? I ended up updating a few more icons, couldn't help
> myself.
Sure, taking a look.

> (In reply to Tim Nguyen :ntim (use needinfo?) from comment #8)
> > Created attachment 8769766 [details]
> > critique helen's icons-1.png
> > I'm seeing some sizing inconsistencies as highlighted in the screenshot, and
> > some shape inconsistencies as well (the shape of both breakpoint icons is
> > different, as well as the { } icon).
> 
> I updated the {} icon to be the same. The debugger icon and the toggle
> breakpoints icon is purposefully different, however.
Looks better.

> Updated the shader editor icon, it was a little blurry.
Looks better as well.

> Made the commandnoautohide icon a little larger, but wasn't sure what to do
> about webaudio and scratchpad—on scratchpad the lines aren't centered if I
> give it one pixel of height, and when I give it two pixels of height it
> looks kinda large. Since both it and webaudio aren't /super/ used, I think
> it's okay to leave them.
I thought the old scratchpad icon looked better sizing-wise, any reason why we can't keep it/pixel-snap it ?

> (In reply to Tim Nguyen :ntim (use needinfo?) from comment #10)
> > Created attachment 8769770 [details]
> > critique-2.png
> I think the stroke seeming large on the filter icon is visual trickery—I
> bumped down the size of the "liquid" inside the filter and I think it helps,
> let me know what you think.
It looks a bit better, but the stroke still feels a bit thick. I've opened the icon with sketch, and it looks like the bottom part of the icon isn't pixel snapped, which is making the stroke blurry.

> (In reply to Tim Nguyen :ntim (use needinfo?) from comment #11)
> > Created attachment 8769772 [details]
> > feedback-3.png
> Updated!
Thanks, these look good!
Comment on attachment 8769853 [details] [diff] [review]
thin-icons.patch

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

r+ for the code changes.
f+ for UI.

Also, I've just realised you've missed 2 toolbar icons: rewind.png and fast-forward.png. Can you update them as well ? I guess you should be able to fork the play icon.

::: devtools/client/themes/images/diff.svg
@@ +3,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg height="16" width="16" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16" fill="whitesmoke">
> +   <path d="M10.2 4.1c-.6 0-1.2.2-1.8.4-.6-.4-1.4-.6-2.2-.6-2.5 0-4.6 2.1-4.6 4.6s2.1 4.6 4.6 4.6c.9 0 1.6-.2 2.3-.7.5.2 1.1.4 1.7.4 2.4 0 4.3-1.9 4.3-4.3.1-2.4-1.9-4.4-4.3-4.4zm-4 7.9c-1.9 0-3.5-1.6-3.5-3.5S4.2 5 6.2 5c.3 0 .7 0 1 .1-.8.9-1.4 2.1-1.4 3.4 0 1.3.6 2.5 1.5 3.3-.4.1-.8.2-1.1.2zm2.1-.7c-.9-.6-1.4-1.6-1.4-2.8 0-1.1.6-2.1 1.4-2.8.8.6 1.4 1.6 1.4 2.8 0 1.1-.6 2.1-1.4 2.8z"/>
> +   <path d="M7.6 8c-.2 0-.3-.1-.4-.2-.1-.2 0-.4.2-.5l1.1-.6c.2-.1.4 0 .5.2.1.2 0 .4-.2.5l-1.1.5c0 .1-.1.1-.1.1zM7.6 9.1c-.1 0-.3-.1-.4-.2-.1-.2 0-.4.2-.5l1.1-.6c.3-.1.5 0 .6.2.1.2 0 .4-.2.5l-1.1.6h-.2zM7.8 10.3c-.1 0-.3-.1-.4-.2-.1-.2 0-.4.2-.5L8.8 9c.2-.1.4 0 .5.2.1.2 0 .4-.2.5l-1.1.6h-.2z"/>
> + </svg>
\ No newline at end of file

nit: leading whitespace

::: devtools/client/themes/images/filter.svg
@@ +1,4 @@
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16" fill="whitesmoke">

The fill was intentionally set to #aaa for this icon, to prevent it from being invisible in the light theme filter boxes (where we can't apply the invert filter)

If you want both colours to be supported, you can use a location hash to designate the wanted colour. You can see how it's done here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/images/commandline-icon.svg

Let me know if you'd like me to do it.
Attachment #8769853 - Flags: review?(ntim.bugs) → review+
Attached image off centre icon.png
The play icon is offcenter. This is not noticeable in the debugger, but it looks a bit odd in the animation inspector.
Attached patch thin-icons.patch (obsolete) — Splinter Review
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #14)
> I thought the old scratchpad icon looked better sizing-wise, any reason why
> we can't keep it/pixel-snap it ?
I made it a little larger—think it's looking pretty similar. We can fudge it another pixel if you still don't like it.

> > (In reply to Tim Nguyen :ntim (use needinfo?) from comment #10)
> It looks a bit better, but the stroke still feels a bit thick. I've opened
> the icon with sketch, and it looks like the bottom part of the icon isn't
> pixel snapped, which is making the stroke blurry.
I think this is fixed now, take a gander.

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #16)
> Created attachment 8769910 [details]
> off centre icon.png
> 
> The play icon is offcenter. This is not noticeable in the debugger, but it
> looks a bit odd in the animation inspector.
Fixed!

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #15)
> Also, I've just realised you've missed 2 toolbar icons: rewind.png and
> fast-forward.png. Can you update them as well ? I guess you should be able
> to fork the play icon.
Added these. Didn't see the fast-forward icon used anywhere in dxr, but this might be worth double-checking.

> ::: devtools/client/themes/images/filter.svg
> @@ +1,4 @@
> >  <!-- This Source Code Form is subject to the terms of the Mozilla Public
> >     - License, v. 2.0. If a copy of the MPL was not distributed with this
> >     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> > +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16" fill="whitesmoke">
> 
> The fill was intentionally set to #aaa for this icon, to prevent it from
> being invisible in the light theme filter boxes (where we can't apply the
> invert filter)
> 
> If you want both colours to be supported, you can use a location hash to
> designate the wanted colour. You can see how it's done here:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/images/
> commandline-icon.svg
> 
> Let me know if you'd like me to do it.
I just set this back to #aaa. I'd like to change some icon colors at some point (I think your point in bug 1268591 is a good one) but I'd rather do it in another patch.
Attachment #8769853 - Attachment is obsolete: true
Attachment #8770245 - Flags: ui-review?(ntim.bugs)
Attachment #8770245 - Flags: review+
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #17)
> Created attachment 8770245 [details] [diff] [review]
> thin-icons.patch
> 
> (In reply to Tim Nguyen :ntim (use needinfo?) from comment #14)
> > I thought the old scratchpad icon looked better sizing-wise, any reason why
> > we can't keep it/pixel-snap it ?
> I made it a little larger—think it's looking pretty similar. We can fudge it
> another pixel if you still don't like it.
It looks better, but I still feel it's smaller than the other ones, maybe it's because the DOM icon next to it is too big ? 

> > > (In reply to Tim Nguyen :ntim (use needinfo?) from comment #10)
> > It looks a bit better, but the stroke still feels a bit thick. I've opened
> > the icon with sketch, and it looks like the bottom part of the icon isn't
> > pixel snapped, which is making the stroke blurry.
> I think this is fixed now, take a gander.
Looks much better, thanks !

> (In reply to Tim Nguyen :ntim (use needinfo?) from comment #16)
> > Created attachment 8769910 [details]
> > off centre icon.png
> > 
> > The play icon is offcenter. This is not noticeable in the debugger, but it
> > looks a bit odd in the animation inspector.
> Fixed!
Thanks! Looks like the pause icon is also a bit off centre, but I'm fine with leaving it that way.

> (In reply to Tim Nguyen :ntim (use needinfo?) from comment #15)
> > Also, I've just realised you've missed 2 toolbar icons: rewind.png and
> > fast-forward.png. Can you update them as well ? I guess you should be able
> > to fork the play icon.
> Added these. Didn't see the fast-forward icon used anywhere in dxr, but this
> might be worth double-checking.
Thanks! the fast-forward icon doesn't seem used, but it doesn't hurt updating it :)
Anyway, I've noticed a pixel snapping issue with the rewind icon.
Attachment #8770245 - Flags: ui-review?(ntim.bugs) → ui-review+
Also, little detail, it would be nice if the | part of the rewind icon has the same size as the | of the pause icon.
Attached patch thin-icons.patchSplinter Review
> > (In reply to Tim Nguyen :ntim (use needinfo?) from comment #16)
> Thanks! Looks like the pause icon is also a bit off centre, but I'm fine
> with leaving it that way.
Fixed!

> > (In reply to Tim Nguyen :ntim (use needinfo?) from comment #15)
> Anyway, I've noticed a pixel snapping issue with the rewind icon.
Fixed this too.

> > (In reply to Tim Nguyen :ntim (use needinfo?) from comment #14)
> It looks better, but I still feel it's smaller than the other ones, maybe
> it's because the DOM icon next to it is too big ? 
Would you like to do another review cycle on this patch for this, or should we file a new bug? I'm kinda okay with this sizing difference personally, but it's up to you.

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #20)
> Also, little detail, it would be nice if the | part of the rewind icon has
> the same size as the | of the pause icon.
Added this as well, I agree.
Attachment #8770245 - Attachment is obsolete: true
Flags: needinfo?(ntim.bugs)
Attachment #8770649 - Flags: ui-review+
Attachment #8770649 - Flags: review+
The scratchpad and webaudio tools are not frequently used, so I'm fine with a new bug.
Flags: needinfo?(ntim.bugs)
Keywords: checkin-needed
Thanks for adressing my issues!
See Also: → 1286624
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #22)
> The scratchpad and webaudio tools are not frequently used, so I'm fine with
> a new bug.
Okeydoke, filed Bug 1286624.

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #23)
> Thanks for adressing my issues!
Thanks for doing all of these reviews!
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/105d425837e7
pixelsnap more toolbar icons and use thin style, r=ntim
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/105d425837e7
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Depends on: 1286913
No longer depends on: 1286913
Depends on: 1287310
Depends on: 1287396
Depends on: 1293322
Depends on: 1296917
I have reproduced this on firefox Nightly according to(2016-03-29)

Fixing bug is verified on Latest Developer Edition-- Build ID: (20160831004001),User Agent: Mozilla/5.0 (Windows NT 10.0; rv:50.0) Gecko/20100101 Firefox/50.0

Tested OS-- Windows10 32bit
QA Whiteboard: [bugday-20160831]
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.