Prettyprint button should be an icon, not a label

RESOLVED FIXED in Firefox 36

Status

DevTools
Debugger
--
minor
RESOLVED FIXED
4 years ago
9 days ago

People

(Reporter: sebo, Assigned: sebo, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 36

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
The pretty-print button within the Debugger panel has a label instead of an icon, which makes styling of it difficult.

So the button should have an icon assigned via CSS like the other buttons within the devtools UI.

FWIW this is needed for Firebug.[1]

Sebastian

[1] https://github.com/firebug/firebug.next/issues/63
Brian, this is simple to fix, but we need an icon...
Any tips what icon we could use?

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #1)
> Brian, this is simple to fix, but we need an icon...
> Any tips what icon we could use?
> 
> Honza

Stephen, I don't see any pretty print icon in the zip attached to Bug 837188.  It's the '{}' button as seen here: https://hacks.mozilla.org/wp-content/uploads/2013/12/pretty-print-minified-JS.png.  We are using text for the button right now, but that makes it harder to style and less consistent across operating systems.  It'd be more consistent if we could use a 1x and 2x png for this, just like debugger-blackbox.png.  Can you help?
Flags: needinfo?(shorlander)
(Assignee)

Comment 3

4 years ago
> It'd be more consistent if we could use a 1x and 2x png for this
General question: Why did you decide for PNGs instead of using SVG icons like Firebug does since version 2.0?

Sebastian
(In reply to Sebastian Zartner from comment #3)
> > It'd be more consistent if we could use a 1x and 2x png for this
> General question: Why did you decide for PNGs instead of using SVG icons
> like Firebug does since version 2.0?
> 
> Sebastian

I started requesting SVG for new icons when we switched to 2x in the blockers for Bug 837188, but for a lot of them it was just easier to stick with png since we already had the images in that format.  I wish we were using SVG for everything because it cleans up the CSS a lot.  For this case SVG would be great, but I think we would also want to do all of the other debugger buttons at the same time if we were to make the switch.
(Assignee)

Comment 5

4 years ago
> but for a lot of them it was just easier to stick with png since we already had the images in that format.
FWIW we also had our icons in PNG format earlier but we took the time to switch everything to SVG.[1]

> I wish we were using SVG for everything because it cleans up the CSS a lot.  For this case SVG would 
> be great, but I think we would also want to do all of the other debugger buttons at the same time if 
> we were to make the switch.
So should I file a bug for switching all icons to SVG?

Sebastian

[1] https://code.google.com/p/fbug/issues/detail?id=7000
(In reply to Sebastian Zartner from comment #5)
> So should I file a bug for switching all icons to SVG?

Sure, probably the most up to date collection of images that need to be converted is from the repository (https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/images/), rather than one of the zips from a previous bug, as we've added and modified quite a few images as one-offs in the meantime.
(Assignee)

Updated

4 years ago
See Also: → bug 1068939

Updated

4 years ago
Blocks: 1070862

Updated

4 years ago
Summary: Don't use label for pretty-print button → Prettyprint button should be an icon, not a label
(Assignee)

Updated

4 years ago
Assignee: nobody → sebastianzartner
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 7

4 years ago
Created attachment 8513906 [details]
debugger-prettyprint.png

Normal resolution prettyprint icon
(Assignee)

Comment 8

4 years ago
Created attachment 8513907 [details]
debugger-prettyprint@2x.png

High resolution prettyprint icon
Flags: needinfo?(shorlander)
(Assignee)

Comment 9

4 years ago
Created attachment 8513912 [details] [diff] [review]
prettyprint.patch

Replace the label by the icon
(Assignee)

Comment 10

4 years ago
This is my very first patch against Mozilla code, so Brian, could you please review and check if I did it right and guide me with the next steps to do?

Note that I had to add my name to the patch manually. Not sure how to get Mercurial to do that for me.

Sebastian
Flags: needinfo?(bgrinstead)
(In reply to Sebastian Zartner from comment #10)
> This is my very first patch against Mozilla code, so Brian, could you please
> review and check if I did it right and guide me with the next steps to do?

Sure!  The patch applies fine, but please add the new image files to the patch with `hg add`.  Also, when you reupload the patch, add r=bgrins at the end of the commit message and upload with me in the r? field.

> Note that I had to add my name to the patch manually. Not sure how to get
> Mercurial to do that for me.

See this document https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F, it will help you get that set up.
Mentor: bgrinstead
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 12

4 years ago
Created attachment 8515125 [details] [diff] [review]
prettyprint2.patch

>> Note that I had to add my name to the patch manually. Not sure how to get
>> Mercurial to do that for me.
>
> See this document https://developer.mozilla.org/en-US
> /docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F, it will help
> you get that set up.

I read that before I uploaded my first patch, though it didn't work. I guess my problem could be related to bug 1077326 as I got the same errors.

Anyway, I hope it works for you now.

Sebastian
Attachment #8513912 - Attachment is obsolete: true
Attachment #8515125 - Flags: review?(bgrinstead)
Comment on attachment 8515125 [details] [diff] [review]
prettyprint2.patch

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

Looks good!  Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=2eac4b0444ff
Attachment #8515125 - Flags: review?(bgrinstead) → review+
(Assignee)

Comment 14

4 years ago
If I interpret those test results correctly, the failing tests are unrelated. (It's a minor layout change, anyway.)
So what's next? Somehow I need to request a check-in, right?

Sebastian
(In reply to Sebastian Zartner from comment #14)
> If I interpret those test results correctly, the failing tests are
> unrelated. (It's a minor layout change, anyway.)
> So what's next? Somehow I need to request a check-in, right?
> 
> Sebastian

Yeah, you can add the checkin-needed keyword to the bug
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/e11192d5f965
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/e11192d5f965
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36

Updated

9 days ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.