Closed Bug 1068381 Opened 5 years ago Closed 5 years ago

Prettyprint button should be an icon, not a label

Categories

(DevTools :: Debugger, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: sebo, Assigned: sebo, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

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)
> 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.
> 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.
See Also: → 1068939
Summary: Don't use label for pretty-print button → Prettyprint button should be an icon, not a label
Assignee: nobody → sebastianzartner
Status: NEW → ASSIGNED
Normal resolution prettyprint icon
High resolution prettyprint icon
Flags: needinfo?(shorlander)
Attached patch prettyprint.patch (obsolete) — Splinter Review
Replace the label by the icon
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)
>> 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+
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e11192d5f965
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.