1216654 - Clean up SVGs in JSON viewer

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect)

Description

[Tim Nguyen :ntim]

SVGO is a great tool for that.

Comment 1

[sjw]

Attachment: bug-1216654-fix.patch

Just took SVGO, let me know if this is enough.

Comment 2

[Tim Nguyen :ntim]

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.

Comment 3

[sjw]

Attachment: bug-1216654-fix-v2.patch

updated

Comment 4

[Brian Grinstead [:bgrins]]

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?

Comment 5

[Tim Nguyen :ntim]

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.

Comment 6

[sjw]

Attachment: bug-1216654-fix-v3.patch

Just updated the patch with Tims feedback.

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Sorry for the delay. New icons look great in the viewer, thank you!

Honza

Comment 8

[sjw]

Comment on attachment 8692455 [details] [diff] [review]
bug-1216654-fix-v3.patch

How can I get access to the try server?

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 10

[Brian Grinstead [:bgrins]]

(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/.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d254257e02b0

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d254257e02b0

Comment 13

[sjw]

Thanks everyone! :)