Tim Nguyen :ntim Reporter
	Description • 8 years ago

We should remove magnifying-glass-*.png and replace its usage with search.svg.

	Ruturaj Vartak Assignee
	Comment 1 • 8 years ago

shall I take it ?

	Tim Nguyen :ntim Reporter
	Comment 2 • 8 years ago

(In reply to Ruturaj Vartak from comment #1)
> shall I take it ?

Please do, I've assigned it to you.

	Ruturaj Vartak Assignee
	Comment 3 • 8 years ago

How do you want me to fork into this ? from master or from the my bug-1253195 branch ?

So, the diffs could either be from master or the HEAD of bug-1253195 branch

	Tim Nguyen :ntim Reporter
	Comment 4 • 8 years ago

The most recently updated branch that contains your change. It landed in mozilla-central, so you should be able to use gecko-dev's master branch.

	Ruturaj Vartak Assignee
	Comment 5 • 8 years ago

The monospace font family in jsonview (linux) isn't loading correctly. Loading the sans-serif system default.
I suspect its https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/common.css#19

:root[platform="linux"] isn't working it seems, Firebug theme works well since its not using that selector.

- Does anybody feel jsonview needs some UX refresh ? The fonts are really tiny

	Tim Nguyen :ntim Reporter
	Comment 6 • 8 years ago

(In reply to Ruturaj Vartak from comment #5)
> Created attachment 8769487 [details]
> json-view-linux.png
> 
> The monospace font family in jsonview (linux) isn't loading correctly.
> Loading the sans-serif system default.
> I suspect its
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/common.
> css#19
> 
> :root[platform="linux"] isn't working it seems, Firebug theme works well
> since its not using that selector.
> 
> - Does anybody feel jsonview needs some UX refresh ? The fonts are really
> tiny

I'd be fine with changes in the Json viewer, but this bug doesn't seem like the right place. Please file a new one for it. I'd be happy to take a look at the changes.

:root[platform="linux"] is not working, because no script sets the attribute. Currently, theme-switching.js takes care of that, but it won't work when loaded in the Json viewer. One thing you can do is setting it from converter-child.js' toHtml function like this: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/theme-switching.js#11

	Ruturaj Vartak Assignee
	Comment 7 • 8 years ago

Hey, Can I assume that we have do away with "images/timeline-filter.svg" and "firebug/timeline-filter.svg" as well ? And keep only references to search.svg and filter.svg

	Tim Nguyen :ntim Reporter
	Comment 8 • 8 years ago

(In reply to Ruturaj Vartak from comment #7)
> Hey, Can I assume that we have do away with "images/timeline-filter.svg" and
> "firebug/timeline-filter.svg" as well ? And keep only references to
> search.svg and filter.svg

Honza gave a green light for this change. You can also remove devtools/client/themes/images/firebug/filter.svg and replace its usage with search.svg.

	Ruturaj Vartak Assignee
	Comment 10 • 8 years ago

forgot to "git rm" unwanted images

	Tim Nguyen :ntim Reporter
	Comment 11 • 8 years ago

I'll review this after bug 1260523 lands.

	Tim Nguyen :ntim Reporter
	Comment 12 • 8 years ago

Comment on attachment 8769995 [details] [diff] [review]
fix-1285747-2.patch

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

The changes look fine to me, only one nit.

You might get conflicts if bug 1260523 lands first.

::: devtools/client/jsonview/css/search-box.css
@@ +9,4 @@
>  .searchBox {
>    height: 18px;
>    font-size: 12px;
> +  background: var(--theme-body-background);

nit: use background-color instead of background

	Ruturaj Vartak Assignee
	Comment 13 • 8 years ago

devtools/client/jsonview/css/search-box.css

-  background: var(--theme-body-background);
+  background-color: var(--theme-body-background);

	Ruturaj Vartak Assignee
	Comment 14 • 8 years ago

I'm seeing a commit in gecko-dev tree

commit 1f0ac9325519f87a0c93d0ff9c8eea841b80bdb8
Author: "Helen V. Holmes" <hholmes@mozilla.com>
Date:   Wed Jul 13 10:42:00 2016 +0200

    Bug 1260523, pixelsnap more toolbar icons and use thin style, r=ntim
    
    MozReview-Commit-ID: 5jOCtKVuhSR


It seems its in "fx-team" branch. Would I have to wait till it comes in "master" ?
I've already submitted a patch however.

	Tim Nguyen :ntim Reporter
	Comment 15 • 8 years ago

(In reply to Ruturaj Vartak from comment #14)
> I'm seeing a commit in gecko-dev tree
> 
> commit 1f0ac9325519f87a0c93d0ff9c8eea841b80bdb8
> Author: "Helen V. Holmes" <hholmes@mozilla.com>
> Date:   Wed Jul 13 10:42:00 2016 +0200
> 
>     Bug 1260523, pixelsnap more toolbar icons and use thin style, r=ntim
>     
>     MozReview-Commit-ID: 5jOCtKVuhSR
> 
> 
> It seems its in "fx-team" branch. Would I have to wait till it comes in
> "master" ?
I think you'll likely get conflicts if you don't wait. But I can take care of those before landing, they should be trivial to fix.

	Tim Nguyen :ntim Reporter
	Comment 16 • 8 years ago

Comment on attachment 8770804 [details] [diff] [review]
fix-1285747-3.patch

Looks fine, can you use the review flag next time you have a patch? This way, I won't miss it.

	Pulsebot
	Comment 17 • 8 years ago

Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/47235b46cc9c
Replace magnifying-glass-*.png with search.svg r=ntim

	Carsten Book [:Tomcat]
	Comment 18 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/47235b46cc9c

	Ruturaj Vartak Assignee
	Comment 19 • 8 years ago

thanks Tim!