Closed Bug 1421491 Opened 3 years ago Closed 3 years ago

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

Categories

(DevTools :: Netmonitor, defect, P3)

defect

Tracking

(firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: victoria, Assigned: sagarbharadwaj50)

Details

Attachments

(2 files, 2 obsolete files)

Attached image Screen Shot 2017-11-28 at 5.35.23 PM.png (obsolete) —
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
Oops - here's a new image that actually shows both problems
Thanks for the report Victoria!

Honza
Priority: -- → P3
UI Analysis doc updated.
Honza
Hello,
I have submitted a mozreview-request for this bug. Please review. Apologies if I have unknowingly broken anything.
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 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
Attachment #8936168 - Attachment is obsolete: true
Hello Honza,
I have updated my patch with the required changes. 
I have squashed everything to one commit.
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+
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?
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
(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)
https://hg.mozilla.org/mozilla-central/rev/9cd0e864b613
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.