Closed Bug 1165576 Opened 6 years ago Closed 6 years ago

Netmonitor theme refresh

Categories

(DevTools :: Netmonitor, defect)

defect
Not set
normal

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: ntim, Assigned: ntim)

References

(Blocks 2 open bugs, )

Details

Attachments

(2 files)

No description provided.
Changes :
- Refreshed table
- Row is now highlighted when hovered (I've seen feedback requesting this)
- Performance report and Sidebar now uses --theme-sidebar-background
- Refreshed footer buttons
- Fixed a bug where the security icon when clicking it.
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Attachment #8606627 - Flags: review?(vporof)
Attachment #8606627 - Flags: review?(bgrinstead)
Attached image Screenshot
Comment on attachment 8606627 [details] [diff] [review]
netmonitor-pretty.patch

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

Deleting code and switching from preprocessor to CSS variables?  Sounds good to me :).  I'll let Victor decide whether we need to send this through a ux review or not.

::: browser/themes/shared/devtools/netmonitor.inc.css
@@ -164,5 @@
>  .requests-security-state-icon:hover {
>    -moz-image-region: rect(0px, 32px, 16px, 16px);
>  }
>  
> -.requests-security-state-icon:active {

Why isn't this needed anymore?

@@ +207,1 @@
>    background-color: transparent;  

Nit: please remove this trailing whitespace while you are here
Attachment #8606627 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #3)
> Comment on attachment 8606627 [details] [diff] [review]
> netmonitor-pretty.patch
> 
> Review of attachment 8606627 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Deleting code and switching from preprocessor to CSS variables?  Sounds good
> to me :).  I'll let Victor decide whether we need to send this through a ux
> review or not.
> 
> ::: browser/themes/shared/devtools/netmonitor.inc.css
> @@ -164,5 @@
> >  .requests-security-state-icon:hover {
> >    -moz-image-region: rect(0px, 32px, 16px, 16px);
> >  }
> >  
> > -.requests-security-state-icon:active {
> 
> Why isn't this needed anymore?
This fixes an issue on OSX where pressing on the security icon would make it disappear.
Flags: needinfo?(vporof)
I'll get to this asap. Sorry about the wait.
Flags: needinfo?(vporof)
Comment on attachment 8606627 [details] [diff] [review]
netmonitor-pretty.patch

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

Very nice, much cleaner!
Attachment #8606627 - Flags: review?(vporof) → review+
Keywords: checkin-needed
Changes look nice and I like the highlight on hover. Just a couple of issues I wanted to point out from my early testing:

I think the white text on black list items on the network panel is a bit hard on the eyes. Looking at https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/DarkTheme-Network@2x.png - the text font color should be grey like the tabs which is better for UX [1]. 

I also notice the reload button isn't really clear that it is a button because of the dark background in the button.

The background colors used seems darker than shorlander's mockup. Was this intentional?

I think changes like this would also really benefit from a ui-review from shorlander prior to landing.

[1] http://uxmovement.com/content/when-to-use-white-text-on-a-dark-background/
Flags: needinfo?(ntim.bugs)
(In reply to Gabriel Luong [:gl] from comment #8)
I tried to minimize the use of custom colors as much as I could, and I have privileged the colors provided by our variables. 
Anyway, can you file a followup and needinfo Stephen on it ?
Flags: needinfo?(ntim.bugs)
See Also: → 1170617
Blocks: 1170617
https://hg.mozilla.org/mozilla-central/rev/3d27abe9852b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Blocks: 1163191
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.