1100135 - DevTools Themes: Replace checkmark symbol with word 'Status' in Netmonitor header

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect)

Description

[Albert Juhé]

From Bug 951714 Comment 68:
> Also, perhaps we can now change the checkmark symbol to read "Status" now?

Comment 1

[Albert Juhé]

Attachment: Screenshot from 2015-12-05 17:13:50.png

Changing the checkmark for the word 'Status' will make that column much wider. That is not a big problem for the normal layout but it is more problematic in the responsive view, where the is less space. Right now, we only display the status icon so the column takes very few space (see screenshot).

Which considerations should we take before working on this bug? We want to make that column much wider in the responsive view? If that is the case, should we also display the status code?

Comment 2

[Tim Nguyen :ntim]

(In reply to Albert Juhé from comment #1)
> Created attachment 8696246 [details]
> Screenshot from 2015-12-05 17:13:50.png
> 
> We want to make that column much wider in the responsive view? If that is the case,
> should we also display the status code?

Yeah, I think it's better to stay consistent for both modes. The status code is essential to have, we should have it in both modes too.

Comment 3

[Albert Juhé]

Attachment: Bug1100135.patch

This patch already works, there is only one thing missing:

>                          data-key="status"
> -                        label="&netmonitorUI.toolbar.status2;"
> +                        label="Status"
>                          flex="1">

What is the correct way to change UI texts? It's my first time modifying one and I don't know how to do it.

Comment 4

[Tim Nguyen :ntim]

(In reply to Albert Juhé from comment #3)
> Created attachment 8697058 [details] [diff] [review]
> Bug1100135.patch
> 
> This patch already works, there is only one thing missing:
> 
> >                          data-key="status"
> > -                        label="&netmonitorUI.toolbar.status2;"
> > +                        label="Status"
> >                          flex="1">
> 
> What is the correct way to change UI texts? It's my first time modifying one
> and I don't know how to do it.

Nope, you need to change the string here (and it's ID (netmonitorUI.toolbar.status2) as well, so localizers can catch the change): https://dxr.mozilla.org/mozilla-central/source/devtools/client/locales/en-US/netmonitor.dtd#27
You can then replace netmonitorUI.toolbar.status2 with the new ID in netmonitor.xul.

Comment 5

[Albert Juhé]

Attachment: Bug1100135.patch

Here it is the patch.

Tim, I marked you as a reviewer, but the mentor of the bug is Victor Porof, not sure which one of you two must review it. Please, correct the flag if it is wrong.

Comment 6

[Tim Nguyen :ntim]

Attachment: Screenshot of patch

Looks great !

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8697362 [details] [diff] [review]
Bug1100135.patch

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

This looks good to me, but :vporof should also look at this since I'm not a peer (you should also change r=ntim to r=vporof,ntim in the commit message).

::: devtools/client/themes/netmonitor.css
@@ +792,4 @@
>    }
>  
> +  .requests-menu-status-code {
> +    width: auto;

Have you tested if this is needed ?

Comment 8

[Albert Juhé]

Attachment: Bug1100135.patch

> This looks good to me, but :vporof should also look at this since I'm not a
> peer (you should also change r=ntim to r=vporof,ntim in the commit message).

Done. I already marked is a r=+ and [checkin-needed].

> Have you tested if this is needed ?

Yes, it is in order to overwrite the 'width: 3em;' set for the "big layout" some lines above.

Maybe a better solution would be to make the "small layout" the default one and a media query for the "big layout"? Anyway, that is out of the scope of this bug.

Comment 10

[Pulsebot]

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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/650a83a0b675

Comment 12

[Khalid Syfullah Zaman [:khalid32]]

I have reproduced this bug on Nightly 36.0a1 (2014-11-16) on ubuntu 14.04 LTS, 32 bit!

The bug's fix is now verified on Latest Nightly 46.0a1!

Build ID: 20160112030227
User Agent: Mozilla/5.0 (X11; Linux i686; rv:46.0) Gecko/20100101 Firefox/46.0

Comment 13

[Rezaul Huque Nayeem]

Reproduced the bug in firefox nightly 36.0a1 (2014-11-16) with windows 10 (64 bit)

Verified as fixed with latest firefox nightly 46.0a1 (Build ID: 20160112030227)

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:46.0) Gecko/20100101 Firefox/46.0 


As it is also verified on Linux (Comment 12), Marking it as verified!