Response headers: minor style fixes needed for selected row, hovered accordion header

RESOLVED FIXED in Firefox 59

Status

()

Firefox
Developer Tools: Netmonitor
P3
normal
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: victoria, Assigned: Sagar Bharadwaj)

Tracking

unspecified
Firefox 59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

2 months ago
Created attachment 8932679 [details]
Screen Shot 2017-11-28 at 5.35.23 PM.png

This could go well with some of the Netmonitor visual polish work for Q1.

Image that shows the bugs attached:

- Selected rows with deep blue background should have white text (the magenta is too hard to read). Right now, if you click the key there's no magenta, but clicking the value or elsewhere in the row causes it to be magenta.

- Accordion header should not turn blue or have underline when hovered
(Reporter)

Comment 1

2 months ago
Created attachment 8932680 [details]
Screen Shot 2017-11-28 at 5.37.28 PM.png

Oops - here's a new image that actually shows both problems
Attachment #8932679 - Attachment is obsolete: true
Thanks for the report Victoria!

Honza
Priority: -- → P3
UI Analysis doc updated.
Honza
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a month ago
Hello,
I have submitted a mozreview-request for this bug. Please review. Apologies if I have unknowingly broken anything.
Comment hidden (mozreview-request)

Comment 7

a month ago
mozreview-review
Comment on attachment 8936166 [details]
Bug 1421491 - Style fixes in Netmonitor

https://reviewboard.mozilla.org/r/206928/#review212802

Thanks for the patch!

Please see my inline comments

Honza

::: devtools/client/netmonitor/src/assets/styles/NetworkDetailsPanel.css:144
(Diff revision 1)
>  
>  .network-monitor .tree-container .treeTable .treeValueCell input{
>    color: var(--string-color);
>  }
>  
> +.treeValueCell input:focus {

Please use more specific selector:
.network-monitor .tree-container .treeTable .treeValueCell input:focus

::: devtools/client/shared/components/tree/TreeView.css:76
(Diff revision 1)
>  }
>  
>  .treeTable .treeRow.selected {
> +  color: var(--theme-selection-color);
>    background-color: var(--theme-selection-background);
>  }

Both changes in this file should go to NetworkDetailsPanel.css and they should also use more precise selector like in the previous case.
Attachment #8936166 - Flags: review?(odvarko) → review-

Comment 8

a month ago
mozreview-review
Comment on attachment 8936168 [details]
Bug 1421491 - Style fixes in Netmonitor

https://reviewboard.mozilla.org/r/206930/#review212804

Please merge this patch with the first one.

Honza
Attachment #8936168 - Flags: review?(odvarko) → review-
Assignee: nobody → sagarbharadwaj50
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8936168 - Attachment is obsolete: true
(Assignee)

Comment 10

a month ago
Hello Honza,
I have updated my patch with the required changes. 
I have squashed everything to one commit.

Comment 11

a month ago
mozreview-review
Comment on attachment 8936166 [details]
Bug 1421491 - Style fixes in Netmonitor

https://reviewboard.mozilla.org/r/206928/#review213140

Looks good, thanks!

R+ 

Honza
Attachment #8936166 - Flags: review?(odvarko) → review+
(Assignee)

Comment 12

a month ago
Hello Honza,

Thanks for your review.
As this is my first patch, I'm not really sure where to go from here. Can you push it to the try server for testing?
(Assignee)

Updated

a month ago
Keywords: checkin-needed
Autoland can't push this patch until all pending issues in MozReview are marked as resolved. Please take care of those and then re-request checkin. Thanks!
Flags: needinfo?(sagarbharadwaj50)
Keywords: checkin-needed

Comment 14

a month ago
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9cd0e864b613
Style fixes in Netmonitor r=Honza
(In reply to Sagar Bharadwaj from comment #12)
> Hello Honza,
> 
> Thanks for your review.
> As this is my first patch, I'm not really sure where to go from here. Can
> you push it to the try server for testing?
Sure, the try push is here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2799f2af1b21
(there is one failure, but unrelated)

I also resolved both issues in MozReview and landed the patch.

You should read about Review board here:
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview.html

Thanks!
Honza
Flags: needinfo?(sagarbharadwaj50)

Comment 16

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9cd0e864b613
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.