Apply new status code design to details panel

RESOLVED FIXED in Firefox 61

Status

P3
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: magicp.jp, Assigned: lucas.lunasouza, Mentored)

Tracking

({good-first-bug})

Trunk
Firefox 61
good-first-bug

Firefox Tracking Flags

(firefox60 wontfix, firefox61 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

a year ago
Posted image status-code.png
Steps to reproduce:
1. Launch Nightly
2. Open Network Monitor (Ctrl+Shift+E)
3. Go to any sites (e.g. https://www.mozilla.org)
4. Select any requests
5. See status code in Headers tab.

Actual results:
Previous design.

Expected results:
Apply new design.
Has STR: --- → yes
Priority: -- → P3
Keywords: good-first-bug
(Assignee)

Comment 1

a year ago
I'm interested in working on this. Is it still available?
(In reply to Lucas Luna Souza from comment #1)
> I'm interested in working on this. Is it still available?
Hi, yes!
Note that you can use "Need more information from" field below, so your request isn't lost.

Since it's been 12 days ago, are you still interested?

Honza
(Assignee)

Comment 3

a year ago
(In reply to Jan Honza Odvarko [:Honza] from comment #2)
> (In reply to Lucas Luna Souza from comment #1)
> > I'm interested in working on this. Is it still available?
> Hi, yes!
> Note that you can use "Need more information from" field below, so your
> request isn't lost.
> 
> Since it's been 12 days ago, are you still interested?
> 
> Honza

Yes, I'm still interested.
Assigned to you!
Honza
Assignee: nobody → ehumphries
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)

Updated

a year ago
Assignee: ehumphries → lucas.lunasouza
(Assignee)

Comment 5

a year ago
Posted patch 1435232.patch (obsolete) — Splinter Review
Sorry for the delay.

I created a patch file to fix this bug. Can you please review it and let me know if anything needs to be changed.
Flags: needinfo?(odvarko)
Attachment #8958947 - Flags: review+
Comment on attachment 8958947 [details] [diff] [review]
1435232.patch

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

Thanks for working on this!

The patch looks good to me, just one general comment.

What about introducing a new component `StatusCode` that would be responsible for rendering the status code and reused by RequestListColumnStatus and HeadersPanel? This way we would avoid code duplication (e.g. calculating the 'code' variable, defining `onMouseOver` handler, markup, etc.). Also `getStatusTooltip` util function could be part of the component.


Btw. you can't set the review to `+` it's up to the reviewer to set it (+ all good, or - changes needed)

Honza

::: devtools/client/netmonitor/src/components/HeadersPanel.js
@@ +17,5 @@
>    getHTTPStatusCodeURL,
>  } = require("../utils/mdn-utils");
> +const { 
> +    getStatusTooltip 
> +} = require("../utils/status-utils");

Remove trailing whitespaces

@@ +183,4 @@
>          urlDetails,
>        },
>      } = this.props;
> +    let statusItem = {fromCache, fromServiceWorker, status, statusText}

Please split this into multiple lines  (every prop at separate line) and fix missing semicolon

@@ +234,4 @@
>              className: "tabpanel-summary-label headers-summary-label",
>            }, SUMMARY_STATUS),
>            div({
> +            className: "requests-list-status-code status-code",

Test:
devtools\client\netmonitor\test\browser_net_status-codes.js

fails for me now. Is it because of this change?

Also, can we remove the `.network-monitor .headers-summary .requests-list-status-icon` rule in NetworkDetailsPanel.css  now?
Attachment #8958947 - Flags: review+
(Assignee)

Comment 8

a year ago
Posted patch 1435232.patch (obsolete) — Splinter Review
Thanks for the feedback. I implemented the changes you suggested and attached a new patch file.

Looks like the test was failing because it was still checking the old css on line 185. I changed that line to check "status-code" instead of "requests-list-status-icon".
Flags: needinfo?(odvarko)
Attachment #8959464 - Flags: review?(odvarko)
Thanks for the update!

Can you please rebase  on top of m-c?
I am seeing couple of collisions.

patching file devtools/client/netmonitor/src/components/HeadersPanel.js
Hunk #1 FAILED at 30
1 out of 4 hunks FAILED -- saving rejects to file devtools/client/netmonitor/src/components/HeadersPanel.js.rej
patching file devtools/client/netmonitor/src/components/RequestListColumnStatus.js
Hunk #2 FAILED at 22
1 out of 2 hunks FAILED -- saving rejects to file devtools/client/netmonitor/src/components/RequestListColumnStatus.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh status-code.patch


Also, you can mark previous version of the patch as obsolete (Details -> Edit Details)

Honza
Flags: needinfo?(odvarko) → needinfo?(lucas.lunasouza)
(Assignee)

Updated

a year ago
Attachment #8958947 - Attachment is obsolete: true
Flags: needinfo?(lucas.lunasouza)
Comment on attachment 8959464 [details] [diff] [review]
1435232.patch

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

Great work here!

A few small inline comments to resolve yet, but the patch already looks good.

Honza

::: devtools/client/netmonitor/src/components/RequestListColumnStatus.js
@@ +11,5 @@
> +// Components
> +
> +loader.lazyGetter(this, "StatusCode", function () {
> +  return createFactory(require("./StatusCode"));
> +});

You don't have to load the StatusCode component lazily. It's immediately used in the render() method anyway. Just require the component directly.

::: devtools/client/netmonitor/src/components/StatusCode.js
@@ +18,5 @@
> +  "status",
> +  "statusText",
> +];
> +
> +class StatusCode extends Component {

Pleas create a comment explaining purpose of the component and where it is used. 

Use the following comment format:

/**
 *
 */

@@ +52,5 @@
> +        or the status-code itself
> +        For example - if a resource is cached, `data-code` would be 200
> +        and the `data-status-code` would be "cached"
> +      */
> +      div({

nit: Use the following comment fomat

//
//
//

... and move the comment before the `return` statement
Attachment #8959464 - Flags: review?(odvarko)
Ah, and the patch still needs to be rebased on top of the latest m-c HEAD.

Honza
(Assignee)

Updated

a year ago
Attachment #8959464 - Attachment is obsolete: true
(Assignee)

Comment 12

a year ago
Posted patch 1435232.patch (obsolete) — Splinter Review
I rebased on top of the latest mozilla-central HEAD, modified the loading of components to not use lazyGetter and added comments to StatusCode.js. Here's the latest patch file.
Flags: needinfo?(odvarko)
Attachment #8960499 - Flags: review?(odvarko)
Comment on attachment 8960499 [details] [diff] [review]
1435232.patch

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

(In reply to Lucas Luna Souza from comment #12)
> I rebased on top of the latest mozilla-central HEAD, modified the loading of
> components to not use lazyGetter and added comments to StatusCode.js. Here's
> the latest patch file.
Looks great now thanks!

I pushed to try and I am seeing some test failures:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=91fa5f4f4433f3c031e99e4c8d83d759b574b3e7

Perhaps it's related to the css/dom changes?

Honza
Attachment #8960499 - Flags: review?(odvarko)
Flags: needinfo?(odvarko)
(Assignee)

Updated

a year ago
Attachment #8960499 - Attachment is obsolete: true
(Assignee)

Comment 14

a year ago
Posted patch 1435232.patchSplinter Review
I updated all of the test cases that were timing out with the new css. Tried it out locally and it seems to be working, none of them are failing anymore for me. New patch file is attached.
Flags: needinfo?(odvarko)
Attachment #8964902 - Flags: review?(odvarko)
Comment on attachment 8964902 [details] [diff] [review]
1435232.patch

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

I pushed to try and it looks good, I see some oranges, but they might be unrelated
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c1ce6c7ebe633a7bab3907f4841e5bbbf7a8ff9

I triggered those again, let's see if they fails.

R+ assuming try is green.

Thanks for the work on this bug!

Honza
Attachment #8964902 - Flags: review?(odvarko) → review+
Don't forget to set checkin-needed flag, so sheriffs can commit it.

Honza
Flags: needinfo?(odvarko) → needinfo?(lucas.lunasouza)
(Assignee)

Updated

a year ago
Flags: needinfo?(lucas.lunasouza)
Attachment #8964902 - Flags: checkin?
Comment on attachment 8964902 [details] [diff] [review]
1435232.patch

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

I pushed to try and it looks good, I see some oranges, but they might be unrelated
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c1ce6c7ebe633a7bab3907f4841e5bbbf7a8ff9

I triggered those again, let's see if they fails.

R+ assuming try is green.

Thanks for the work on this bug!

Honza
Attachment #8964902 - Flags: checkin?
@Lucas: sorry for not being clear, checkin-needed is a keyword that needs to be added.
I am doing it for you now, but you can read about the process here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree


Thanks for the help on this bug!

Honza
Keywords: checkin-needed

Comment 19

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da1badd944ab
Apply new status code design to details panel. r=Honza
Keywords: checkin-needed

Comment 20

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/da1badd944ab
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
status-firefox60: affected → wontfix

Updated

9 months ago
Product: Firefox → DevTools

Comment 21

9 months ago
Was the status code text supposed to be removed in the "Status code:" line for 61.0? https://imgur.com/a/FEGzXtj
Flags: needinfo?(lucas.lunasouza)

Comment 22

9 months ago
I've opened a new bug report regarding the missing response text https://bugzilla.mozilla.org/show_bug.cgi?id=1472875
Flags: needinfo?(lucas.lunasouza)
@Adam: thanks for the report, the status text wasn't supposed to go away.
Honza

Updated

9 months ago
Depends on: 1472875
You need to log in before you can comment on or make changes to this bug.