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

VERIFIED FIXED in Firefox 46

Status

()

Firefox
Developer Tools: Netmonitor
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: Albert Juhé, Assigned: Albert Juhé, Mentored)

Tracking

Trunk
Firefox 46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 verified)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
From Bug 951714 Comment 68:
> Also, perhaps we can now change the checkmark symbol to read "Status" now?
(Assignee)

Updated

3 years ago
Assignee: nobody → aljullu
Depends on: 951714
Version: unspecified → Trunk

Updated

3 years ago
No longer depends on: 951714

Updated

3 years ago
Depends on: 951714
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
Created attachment 8696246 [details]
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?
Flags: needinfo?(ntim.bugs)

Comment 2

2 years ago
(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.
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 3

2 years ago
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.
Attachment #8697058 - Flags: feedback?(ntim.bugs)

Comment 4

2 years ago
(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.

Updated

2 years ago
Attachment #8697058 - Flags: feedback?(ntim.bugs)
(Assignee)

Comment 5

2 years ago
Created attachment 8697362 [details] [diff] [review]
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.
Attachment #8697058 - Attachment is obsolete: true
Attachment #8697362 - Flags: review?(ntim.bugs)

Comment 6

2 years ago
Created attachment 8697660 [details]
Screenshot of patch

Looks great !

Comment 7

2 years ago
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 ?
Attachment #8697362 - Flags: review?(vporof)
Attachment #8697362 - Flags: review?(ntim.bugs)
Attachment #8697362 - Flags: review+
Attachment #8697362 - Flags: review?(vporof) → review+
(Assignee)

Comment 8

2 years ago
Created attachment 8697678 [details] [diff] [review]
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.
Attachment #8697362 - Attachment is obsolete: true
Attachment #8697678 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1048837

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/650a83a0b675
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
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
QA Whiteboard: [bugday-20160113]

Comment 13

2 years ago
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!
Status: RESOLVED → VERIFIED
status-firefox46: fixed → verified
You need to log in before you can comment on or make changes to this bug.