1435232 - Apply new status code design to details panel

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[magicp]

Attachment: 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.

Comment 1

[Lucas Luna Souza]

I'm interested in working on this. Is it still available?

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 3

[Lucas Luna Souza]

(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.

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Assigned to you!
Honza

Comment 5

[Lucas Luna Souza]

Attachment: 1435232.patch

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.

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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?

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=96d3d4537fa177b41fe0624c883355d9530134c6

See the eslint errors.

Honza

Comment 8

[Lucas Luna Souza]

Attachment: 1435232.patch

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".

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Ah, and the patch still needs to be rebased on top of the latest m-c HEAD.

Honza

Comment 12

[Lucas Luna Souza]

Attachment: 1435232.patch

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.

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 14

[Lucas Luna Souza]

Attachment: 1435232.patch

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.

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 16

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Don't forget to set checkin-needed flag, so sheriffs can commit it.

Honza

Comment 17

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da1badd944ab
Apply new status code design to details panel. r=Honza

Comment 20

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/da1badd944ab

Comment 21

[Adam]

Was the status code text supposed to be removed in the "Status code:" line for 61.0? https://imgur.com/a/FEGzXtj

Comment 22

[Adam]

I've opened a new bug report regarding the missing response text https://bugzilla.mozilla.org/show_bug.cgi?id=1472875

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Adam: thanks for the report, the status text wasn't supposed to go away.
Honza