Closed Bug 1265985 Opened 4 years ago Closed 4 years ago

Create toolbox icons for Firebug theme

Categories

(DevTools :: General, enhancement)

enhancement
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: sebo, Assigned: Honza)

References

Details

Attachments

(12 files, 16 obsolete files)

16.79 KB, image/png
Details
2.95 KB, image/svg+xml
Details
2.30 KB, image/svg+xml
Details
1.25 KB, image/svg+xml
Details
1.47 KB, image/svg+xml
Details
3.28 KB, image/svg+xml
Details
2.78 KB, image/svg+xml
Details
1.24 KB, image/svg+xml
Details
2.59 KB, image/svg+xml
Details
2.76 KB, image/svg+xml
Details
1.90 KB, image/svg+xml
Details
119.14 KB, patch
Honza
: review+
Details | Diff | Splinter Review
The Firebug theme from bug 1244054 misses some theme-specific icons for the toolbox buttons.
This bug targets to add them.

Sebastian
Attached image Split console icon (obsolete) —
This icon was initially used by Firebug 2.

Sebastian
Attached image Responsive Design Mode icon (obsolete) —
Attached image iframe Selector icon (obsolete) —
These three icons are the ones shown by default.

I'll try to create the others before 48.0 reaches DevEdition, but those can already be integrated in the meantime.
Honza, can you do this?

Sebastian
Flags: needinfo?(odvarko)
Attached image Eyedropper icon (obsolete) —
Attached image Highlight Painted Areas icon (obsolete) —
Attached image Scratchpad icon (obsolete) —
Attached patch bug1265985-icons.patch (obsolete) — Splinter Review
Attaching a patch that uses new icons in the Firebug theme

Missing icons:
* theme/images/firebug/command-screenshot.svg
* theme/images/firebug/command-rulers.svg
* theme/images/firebug/command-measure.svg

This one in only for the browser toolbox:
* theme/images/firebug/command-noautohide.svg


@Sebastian: command-eyedropper.svg is almost invisible in the background. I'll attach a screenshot.

@Helen: do you think we can commit just some icons or wait till all are available? Note that hitting 48 would be great.

@ntim: I created the black list for Firebug icons (you've suggested) and removed the 'filter: none' from toolbars.css variables. How does it look to you now?

Honza
Assignee: nobody → odvarko
Flags: needinfo?(sebastianzartner)
Flags: needinfo?(odvarko)
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(hholmes)
Attachment #8743710 - Flags: feedback?(ntim.bugs)
Attached image Eyedropper icon (obsolete) —
Made the borders a bit darker and added a filling. Let me know if it's not enough.

Sebastian
Attachment #8743250 - Attachment is obsolete: true
Flags: needinfo?(sebastianzartner)
(In reply to Jan Honza Odvarko [:Honza] from comment #8)
> Missing icons:
> * theme/images/firebug/command-screenshot.svg
> * theme/images/firebug/command-rulers.svg
> * theme/images/firebug/command-measure.svg
> 
> This one in only for the browser toolbox:
> * theme/images/firebug/command-noautohide.svg

I'll try to get those today, so we still get them in time for 48.

Sebastian
Attached image Screenshot icon (obsolete) —
Attached image Measure icon (obsolete) —
Attached image Rulers icon (obsolete) —
Attached image Disable popup auto-hide icon (obsolete) —
Those should be all icons, so I'm removing the needinfo request for Helen.

Sebastian
Flags: needinfo?(hholmes)
(In reply to Sebastian Zartner [:sebo] from comment #16)
> Those should be all icons, so I'm removing the needinfo request for Helen.
> 
> Sebastian

Thanks Sebastian!
Attached patch bug1265985-icons.patch (obsolete) — Splinter Review
Great, here is a new patch.
Helen, what do you think?

Honza
Attachment #8743710 - Attachment is obsolete: true
Attachment #8743728 - Attachment is obsolete: true
Attachment #8743710 - Flags: feedback?(ntim.bugs)
Attachment #8743859 - Flags: review?(hholmes)
Attached image command-buttons.png
A screenshot of the UI in action.

Honza
Attachment #8743859 - Flags: review?(hholmes) → review+
Thanks Helen!

Honza
Keywords: checkin-needed
Comment on attachment 8743859 [details] [diff] [review]
bug1265985-icons.patch

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

Can you run all SVGs through SVGO please (and make sure to re-add the license header after that, since SVGO removes it) ?

Thanks !

::: devtools/client/inspector/inspector-panel.js
@@ +176,5 @@
> +      label: "test",
> +      callback: function () {
> +        Cmds.showTroubleShooting();
> +      }
> +    }];

Why is it needed ?

Also, this needs to be indented properly

::: devtools/client/themes/toolbars.css
@@ +73,3 @@
>    --tool-options-image: url(images/firebug/tool-options.svg);
>    --close-button-image: url(chrome://devtools/skin/images/firebug/close.svg);
> +  --icon-filter: invert(1);

Thanks for taking care of this here !
Attachment #8743859 - Flags: feedback-
Flags: needinfo?(ntim.bugs)
Keywords: checkin-needed
Attached image Eyedropper icon
Attachment #8743739 - Attachment is obsolete: true
Attachment #8743252 - Attachment is obsolete: true
Attached image iframe Selector icon
Attachment #8743241 - Attachment is obsolete: true
Attached image Measure icon
Attachment #8743788 - Attachment is obsolete: true
Attachment #8743803 - Attachment is obsolete: true
Attachment #8743238 - Attachment is obsolete: true
Attached image Rulers icon
Attachment #8743789 - Attachment is obsolete: true
Attached image Scratchpad icon
Attachment #8743519 - Attachment is obsolete: true
Attached image Screenshot icon
Attachment #8743763 - Attachment is obsolete: true
Attached image Split console icon
Attachment #8743237 - Attachment is obsolete: true
(In reply to Tim Nguyen [:ntim] (busy, email me instead) from comment #21)
> Comment on attachment 8743859 [details] [diff] [review]
> bug1265985-icons.patch
> 
> Review of attachment 8743859 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you run all SVGs through SVGO please (and make sure to re-add the
> license header after that, since SVGO removes it) ?

Done and attached the new files now.

Sebastian
Attached patch bug1265985-icons.patch (obsolete) — Splinter Review
(In reply to Tim Nguyen [:ntim] (busy, email me instead) from comment #21)
> Why is it needed ?
Ah, good catch, removed.

@Sebastian: thanks for the icon update. Also, not big deal, but if you use the same file names for images as in the tree, it's easier to deal with it.

New patch attached.
Honza
Attachment #8743859 - Attachment is obsolete: true
Attachment #8744194 - Flags: feedback?(ntim.bugs)
Comment on attachment 8744194 [details] [diff] [review]
bug1265985-icons.patch

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

I have a new version of this patch that addresses both comments.

::: devtools/client/themes/firebug-theme.css
@@ +16,5 @@
> +
> +.theme-firebug #toolbox-dock-buttons > toolbarbutton > image,
> +.theme-firebug .devtools-closebutton > image,
> +.theme-firebug .devtools-option-toolbarbutton > image,
> +.theme-firebug .devtools-toolbarbutton > image,

This line should be removed, otherwise a lot of icons will have low contrast (clear/perf/network monitor icons/...).

::: devtools/client/themes/images/firebug/command-console.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" xmlns:xlink="http://www.w3.org/1999/xlink" width="16" height="16"><defs><linearGradient id="a"><stop offset="0" stop-color="#325de6"/><stop offset="1" stop-color="#8ba3f1"/></linearGradient><linearGradient id="b"><stop offset="0" stop-color="#1a47d6"/><stop offset="1" stop-color="#6786ed"/></linearGradient><linearGradient x1="7.771" y1="13.61" x2="7.771" y2="2.16" id="e" xlink:href="#a" gradientUnits="userSpaceOnUse" gradientTransform="matrix(-.70047 0 0 .8 8.145 1037.962)"/><linearGradient x1="7.21" y1="14.919" x2="7.21" y2="1.081" id="f" xlink:href="#b" gradientUnits="userSpaceOnUse" gradientTransform="matrix(-.70047 0 0 .8 8.145 1037.962)"/><linearGradient id="c"><stop offset="0" stop-color="#282828"/><stop offset="1" stop-color="#505050"/></linearGradient><linearGradient id="d"><stop offset="0"/><stop offset="1" stop-color="#3c3c3c"/></linearGradient><linearGradient x1="7.771" y1="15.451" x2="7.771" y2="3.941" id="g" xlink:href="#c" gradientUnits="userSpaceOnUse" gradientTransform="matrix(-.9204 0 0 .56 15.804 1039.472)"/><linearGradient x1="7.21" y1="16.411" x2="7.21" y2="3.037" id="h" xlink:href="#d" gradientUnits="userSpaceOnUse" gradientTransform="matrix(-.9204 0 0 .56 15.804 1039.472)"/></defs><g stroke-linejoin="round"><path d="M1.14 1039.162l4.56 5.167-4.56 5.233-.84-.8 3.166-4.433-3.166-4.367z" fill="url(#e)" stroke="url(#f)" stroke-width=".6" transform="translate(0 -1036.362)"/><path d="M5.688 1040.05v1.125h10.125v-1.125H5.687zm2.5 3.75v1.125h7.624v-1.125H8.189zm-2.5 3.75v1.125h10.125v-1.125H5.687z" style="marker:none" color="#000" fill="url(#g)" stroke="url(#h)" stroke-width=".4" overflow="visible" transform="translate(0 -1036.362)"/></g></svg>
\ No newline at end of file

The SVGs are probably *too* optimised here. Anyway, I have a new version of this patch that prettifies up those SVGs (and sets the indent to 2 spaces on other SVGs)
Attachment #8744194 - Flags: feedback?(ntim.bugs) → feedback+
Attached patch bug1265985-icons.patch (obsolete) — Splinter Review
Here's the patch with both comments addressed.
Now properly inverts icons
Attachment #8744194 - Attachment is obsolete: true
Attachment #8744253 - Attachment is obsolete: true
Attachment #8744263 - Flags: review?(odvarko)
Comment on attachment 8744263 [details] [diff] [review]
bug1265985-icons.patch

Looks good to me now.

You can land it, thanks!

Honza
Attachment #8744263 - Flags: review?(odvarko) → review+
(In reply to Jan Honza Odvarko [:Honza] from comment #33)
> Created attachment 8744194 [details] [diff] [review]
> bug1265985-icons.patch
> 
> (In reply to Tim Nguyen [:ntim] (busy, email me instead) from comment #21)
> > Why is it needed ?
> Ah, good catch, removed.
> 
> @Sebastian: thanks for the icon update. Also, not big deal, but if you use
> the same file names for images as in the tree, it's easier to deal with it.

I'll keep that in mind next time. (There are still icons throughout the UI, which may be replaced by custom ones.)

(In reply to Tim Nguyen [:ntim] (busy, email me instead) from comment #34)
> ::: devtools/client/themes/images/firebug/command-console.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" xmlns:xlink="http://www.w3.org/1999/xlink" width="16" height="16"><defs><linearGradient id="a"><stop offset="0" stop-color="#325de6"/><stop offset="1" stop-color="#8ba3f1"/></linearGradient><linearGradient id="b"><stop offset="0" stop-color="#1a47d6"/><stop offset="1" stop-color="#6786ed"/></linearGradient><linearGradient x1="7.771" y1="13.61" x2="7.771" y2="2.16" id="e" xlink:href="#a" gradientUnits="userSpaceOnUse" gradientTransform="matrix(-.70047 0 0 .8 8.145 1037.962)"/><linearGradient x1="7.21" y1="14.919" x2="7.21" y2="1.081" id="f" xlink:href="#b" gradientUnits="userSpaceOnUse" gradientTransform="matrix(-.70047 0 0 .8 8.145 1037.962)"/><linearGradient id="c"><stop offset="0" stop-color="#282828"/><stop offset="1" stop-color="#505050"/></linearGradient><linearGradient id="d"><stop offset="0"/><stop offset="1" stop-color="#3c3c3c"/></linearGradient><linearGradient x1="7.771" y1="15.451" x2="7.771" y2="3.941" id="g" xlink:href="#c" gradientUnits="userSpaceOnUse" gradientTransform="matrix(-.9204 0 0 .56 15.804 1039.472)"/><linearGradient x1="7.21" y1="16.411" x2="7.21" y2="3.037" id="h" xlink:href="#d" gradientUnits="userSpaceOnUse" gradientTransform="matrix(-.9204 0 0 .56 15.804 1039.472)"/></defs><g stroke-linejoin="round"><path d="M1.14 1039.162l4.56 5.167-4.56 5.233-.84-.8 3.166-4.433-3.166-4.367z" fill="url(#e)" stroke="url(#f)" stroke-width=".6" transform="translate(0 -1036.362)"/><path d="M5.688 1040.05v1.125h10.125v-1.125H5.687zm2.5 3.75v1.125h7.624v-1.125H8.189zm-2.5 3.75v1.125h10.125v-1.125H5.687z" style="marker:none" color="#000" fill="url(#g)" stroke="url(#h)" stroke-width=".4" overflow="visible" transform="translate(0 -1036.362)"/></g></svg>
> \ No newline at end of file
> 
> The SVGs are probably *too* optimised here. Anyway, I have a new version of
> this patch that prettifies up those SVGs (and sets the indent to 2 spaces on
> other SVGs)

It was the first time I used SVGO, so bear with me. Thanks for updating the patch accordingly!

I'd say that was a very good teamwork! :-)

Sebastian
Duplicate of this bug: 1266305
Duplicate of this bug: 1266306
Duplicate of this bug: 1266308
https://hg.mozilla.org/mozilla-central/rev/0361b7030d36
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Depends on: 1300719
Depends on: 1301042
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.