Closed Bug 1216654 Opened 4 years ago Closed 4 years ago

Clean up SVGs in JSON viewer

Categories

(DevTools :: CSS and Themes, defect)

defect
Not set

Tracking

(firefox45 fixed)

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: ntim, Assigned: sjw+bugzilla)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

SVGO is a great tool for that.
Attached patch bug-1216654-fix.patch (obsolete) — Splinter Review
Just took SVGO, let me know if this is enough.
Assignee: nobody → sjw
Status: NEW → ASSIGNED
Attachment #8690425 - Flags: review?(bgrinstead)
Comment on attachment 8690425 [details] [diff] [review]
bug-1216654-fix.patch

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

::: devtools/client/jsonview/css/read-only-prop.svg
@@ +2,3 @@
>  <!-- 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/. -->

nit: we usually put the SVG tag after the license header. (applies to other files too). Looks good to me otherwise.
Attached patch bug-1216654-fix-v2.patch (obsolete) — Splinter Review
updated
Attachment #8690425 - Attachment is obsolete: true
Attachment #8690425 - Flags: review?(bgrinstead)
Attachment #8690439 - Flags: review?(bgrinstead)
Comment on attachment 8690439 [details] [diff] [review]
bug-1216654-fix-v2.patch

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

LGTM.  Honza, can you check and make sure the json viewer UI still looks right with the icon update here?
Attachment #8690439 - Flags: review?(odvarko)
Attachment #8690439 - Flags: review?(bgrinstead)
Attachment #8690439 - Flags: review+
Comment on attachment 8690439 [details] [diff] [review]
bug-1216654-fix-v2.patch

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

Just got time to make a more complete review.

2 main comments about this patch:
- Can you configure SVGO to round numbers as well (a precision of 10^-3 should be enough) ?
- It'd be nice if you could get SVGO to apply flatten transforms as well, you can usually do that by moving down the transform to the children if it's currently applied on a <g> tag. (if you don't have time to do this one, it's fine)

::: devtools/client/jsonview/css/search.svg
@@ +9,4 @@
>      </linearGradient>
> +    <linearGradient id="b">
> +      <stop offset="0" stop-color="#2f5d93"/>
> +      <stop offset="1" stop-color="#3a87bd" stop-opacity="0.99215686"/>

The stop-opacity here is basically 1. You can omit the stop-opacity attribute here.
Just updated the patch with Tims feedback.
Attachment #8692455 - Flags: review?(odvarko)
Attachment #8690439 - Attachment is obsolete: true
Attachment #8690439 - Flags: review?(odvarko)
Sorry for the delay. New icons look great in the viewer, thank you!

Honza
Attachment #8692455 - Flags: review?(odvarko) → review+
Attachment #8692455 - Flags: checkin?
Comment on attachment 8692455 [details] [diff] [review]
bug-1216654-fix-v3.patch

How can I get access to the try server?
Attachment #8692455 - Flags: checkin?
(In reply to sjw from comment #8)
> Comment on attachment 8692455 [details] [diff] [review]
> bug-1216654-fix-v3.patch
> 
> How can I get access to the try server?

https://wiki.mozilla.org/ReleaseEngineering/TryServer#Try_Server

Honza
(In reply to sjw from comment #8)
> Comment on attachment 8692455 [details] [diff] [review]
> bug-1216654-fix-v3.patch
> 
> How can I get access to the try server?

For this change we shouldn't need a try push so I'll just mark checkin-needed.  To get access to the try server you will need to apply for level 1 commit access as described in the procedure here: https://www.mozilla.org/en-US/about/governance/policies/commit/.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d254257e02b0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Thanks everyone! :)
Product: Firefox → DevTools
Component: JSON Viewer → CSS and Themes
You need to log in before you can comment on or make changes to this bug.