Closed Bug 1285747 Opened 3 years ago Closed 3 years ago

Replace magnifying-glass-*.png with search.svg

Categories

(DevTools :: Framework, enhancement)

enhancement
Not set

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: ntim, Assigned: ruturaj)

References

Details

(Whiteboard: [good first bug][lang=css])

Attachments

(2 files, 2 obsolete files)

We should remove magnifying-glass-*.png and replace its usage with search.svg.
shall I take it ?
Flags: needinfo?(ntim.bugs)
(In reply to Ruturaj Vartak from comment #1)
> shall I take it ?

Please do, I've assigned it to you.
Assignee: nobody → ruturaj
Status: NEW → ASSIGNED
Flags: needinfo?(ntim.bugs)
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
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.
Attached image 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
Flags: needinfo?(ntim.bugs)
(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
Flags: needinfo?(ntim.bugs)
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
Flags: needinfo?(ntim.bugs)
(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.
Flags: needinfo?(ntim.bugs)
Attached patch fix-1285747-1.patch (obsolete) — Splinter Review
Attached patch fix-1285747-2.patch (obsolete) — Splinter Review
forgot to "git rm" unwanted images
Attachment #8769994 - Attachment is obsolete: true
Severity: normal → enhancement
I'll review this after bug 1260523 lands.
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
Attachment #8769995 - Flags: review+
devtools/client/jsonview/css/search-box.css

-  background: var(--theme-body-background);
+  background-color: var(--theme-body-background);
Attachment #8769995 - Attachment is obsolete: true
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.
Flags: needinfo?(ntim.bugs)
(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.
Flags: needinfo?(ntim.bugs)
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.
Attachment #8770804 - Flags: review+
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/47235b46cc9c
Replace magnifying-glass-*.png with search.svg r=ntim
https://hg.mozilla.org/mozilla-central/rev/47235b46cc9c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
thanks Tim!
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.