Closed Bug 1324533 Opened 7 years ago Closed 7 years ago

link timings panel to explanation

Categories

(DevTools :: Netmonitor, defect, P1)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(firefox55 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Iteration:
55.3 - Apr 17
Tracking Flags:
Tracking Status
firefox55 --- verified

People

(Reporter: tromey, Assigned: locke12456, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [netmonitor-reserve])

Attachments

(2 files, 2 obsolete files)

Bug 1324533 - link timings panel to explanation.
59 bytes, text/x-review-board-request
ntim
: review+
rickychien
: review+
Details
headers-learn-more-links.png
15.27 KB, image/png
Details 
I happened to notice that in the Chrome devtools network panel, their
equivalent of the timings sub-panel has a link to a web site documenting
the meanings of the various timings.  This seemed like a nice detail to me.
Ours could link to https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Timings

Perhaps the security panel could also link to this MDN page.
Keywords: good-first-bug
Hi Tom,
That sounds great. I think we need to touch the html structure here.
I can see a bunch of divs representing a table like view there.
How do you want this to be implemented ? I am interested in working on this with you.

Thanks
Flags: needinfo?(ttromey)
(In reply to [:Towkir] Ahmed from comment #1)

> That sounds great. I think we need to touch the html structure here.
> I can see a bunch of divs representing a table like view there.
> How do you want this to be implemented ? I am interested in working on this
> with you.

I think just a link below the timings chart would be good.
It could work similarly to the MDN link that's in the rule view.
The code for that is here:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/widgets/MdnDocsWidget.js#273
Flags: needinfo?(ttromey)
Hi Tom,
It would be nice if you could specify the html/xul file I should touch for this.
I will submit a patch soon.

Thanks
Assignee: nobody → 3ugzilla
Status: NEW → ASSIGNED
Flags: needinfo?(ttromey)
You'll want to change return div({}, timelines); to return div({}, timelines, mdnLink); at [0].

In the render function, you'll also need to define mdnLink to be an element that would open [1] in the current tab, [2] shows you how to open a link in the current tab.

[0]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/shared/components/timings-panel.js#61

[1]: https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Timings

[2]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/widgets/MdnDocsWidget.js#273
Just setting the priority field.

Thanks for working on this Towkir!

Honza
Priority: -- → P3
I think :ntim answered everything, so I'm clearing the NI.
Thanks for looking at this.
Flags: needinfo?(ttromey)
Hi Ahmed, would you stil like to take a look on this?
Blocks: netmonitor-html
Flags: needinfo?(3ugzilla)
Whiteboard: [netmonitor]
Flags: qe-verify?
Whiteboard: [netmonitor] → [netmonitor-reserve]
Hi Fred,
I am currently a bit busy, but I will be able to resume working again within one week or so,
But if there is any urgency, you can go ahead :)
Thanks
Flags: needinfo?(3ugzilla)
Flags: qe-verify? → qe-verify+
QA Contact: ciprian.georgiu
Ahmed, no problem, take your time :)
Blocks: netmonitor-phaseII
No longer blocks: netmonitor-html
Hi Ahmed, could you arrange time for solving this? Or maybe we can put this bug back for somebody to take?
Flags: needinfo?(3ugzilla)
I don't think I will be able to manage time. Leaving this open for others. :)
Assignee: 3ugzilla → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(3ugzilla)
Assignee: nobody → locke12456
Mentor: gasolin
Status: NEW → ASSIGNED
I traced the code for all the components and found that I could re-use the method called MDNLink.
Could I just use it?
https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/src/components/mdn-link.js#19
Flags: needinfo?(odvarko)
(In reply to Locke Chen from comment #12)
> I traced the code for all the components and found that I could re-use the
> method called MDNLink.
> Could I just use it?
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/
> src/components/mdn-link.js#19

Yes, you should be able to re-use that.
Flags: needinfo?(odvarko)
Comment on attachment 8853895 [details]
Bug 1324533 - link timings panel to explanation.

https://reviewboard.mozilla.org/r/125930/#review128416

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:568
(Diff revision 1)
>  
>  .requests-list-timings-box.receive {
>    background-color: var(--timing-receive-color);
>  }
>  
> +

Seems like you've introduced a new line here somehow, can you remove it ?

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:1133
(Diff revision 1)
>    outline: 0;
>    box-shadow: var(--theme-focus-box-shadow-textbox);
>  }
>  
>  .panel-container,
> +

Same here.

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:1292
(Diff revision 1)
> +.learn-more-link {
> +  color: var(--theme-highlight-blue);
> +  cursor: pointer;
> +  margin: 0 5px;
> +  white-space: nowrap;
> +  flex-grow: 1;
> +}
> +
> +.learn-more-link:hover {
> +  text-decoration: underline;
> +}

It's better to remove .headers-panel from these rules rather than copying them.

https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/src/assets/styles/netmonitor.css#681

::: devtools/client/netmonitor/src/components/timings-panel.js:63
(Diff revision 1)
> -
> -  return div({ className: "panel-container" }, timelines);
> +  const mdnLink = MDNLink({
> +            url: TIMINGS_MDN_URL,
> +          });

nit: can you fix the indentation?

const mdnLink = MDNLink({
  url: TIMINGS_MDN_URL,
});

::: devtools/client/netmonitor/src/components/timings-panel.js:66
(Diff revision 1)
>      );
>    });
> -
> -  return div({ className: "panel-container" }, timelines);
> +  const mdnLink = MDNLink({
> +            url: TIMINGS_MDN_URL,
> +          });
> +  return div({ className: "panel-container" }, timelines,mdnLink

nit: space after `timelines,`
Attachment #8853895 - Flags: review?(ntim.bugs)
Hi Tim
I fixed your hints. Could you please review my commit again?
Thank you. :)
Flags: needinfo?(ntim.bugs)
Attachment #8853895 - Flags: review?(rchien)
Comment on attachment 8853895 [details]
Bug 1324533 - link timings panel to explanation.

https://reviewboard.mozilla.org/r/125930/#review129176

::: devtools/client/netmonitor/src/components/timings-panel.js:14
(Diff revision 3)
>  
>  const { div, span } = DOM;
>  const types = ["blocked", "dns", "connect", "send", "wait", "receive"];
>  const TIMINGS_END_PADDING = "80px";
>  
> +const TIMINGS_MDN_URL = "https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Timings";

Please move the URL link retrival to `utils/mdn-utils`, that's where we collect all MDN related links

::: devtools/client/netmonitor/src/components/timings-panel.js:66
(Diff revision 3)
>      );
>    });
> -
> -  return div({ className: "panel-container" }, timelines);
> +  const mdnLink = MDNLink({
> +    url: TIMINGS_MDN_URL,
> +  });
> +  return div({ className: "panel-container" }, timelines, mdnLink

you can put MDNLink directly inside of return statement, and save the extra mdnlink variable

```
return div({ className: "panel-container" },
  timelines,
  MDNLink({
    url: TIMINGS_MDN_URL,
  })
```
Comment on attachment 8853895 [details]
Bug 1324533 - link timings panel to explanation.

https://reviewboard.mozilla.org/r/125930/#review129186

::: devtools/client/netmonitor/src/components/timings-panel.js:7
(Diff revision 3)
>  const { DOM, PropTypes } = require("devtools/client/shared/vendor/react");
>  const { L10N } = require("../utils/l10n");
>  
>  const { div, span } = DOM;
>  const types = ["blocked", "dns", "connect", "send", "wait", "receive"];
>  const TIMINGS_END_PADDING = "80px";
>  
> +const TIMINGS_MDN_URL = "https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Timings";
> +const MDNLink = require("./mdn-link");

nit: Let's do some coding style refinement here.

For an example: https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/monitor-panel.js#7-28

We have a loose coding style convetion in react component like above example.

module require order is:

1. shared lib/framework (e.g. "devtools/client/shared/xxx")
2. netmonitor's lib/framework (e.g. "./utils/xxx")
3. shared components
4. netmonitor's components
5. react DOM alias (e.g. { div } = DOM;)
6. constants or others

so I suggest:

```
const { DOM, PropTypes } = require("devtools/client/shared/vendor/react");
const { L10N } = require("../utils/l10n");

// Components
const MDNLink = require("./mdn-link");

const { div, span } = DOM;
const types = ["blocked", "dns", "connect", "send", "wait", "receive"];
const TIMINGS_END_PADDING = "80px";


``

::: devtools/client/netmonitor/src/components/timings-panel.js:14
(Diff revision 3)
>  
>  const { div, span } = DOM;
>  const types = ["blocked", "dns", "connect", "send", "wait", "receive"];
>  const TIMINGS_END_PADDING = "80px";
>  
> +const TIMINGS_MDN_URL = "https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Timings";

nit: and please remove en-US/ portion in url. MDN website will redirect language for user depends on where they are by detecting their IP address.

::: devtools/client/netmonitor/src/components/timings-panel.js:66
(Diff revision 3)
>      );
>    });
> -
> -  return div({ className: "panel-container" }, timelines);
> +  const mdnLink = MDNLink({
> +    url: TIMINGS_MDN_URL,
> +  });
> +  return div({ className: "panel-container" }, timelines, mdnLink

As Fred mentioned, putting MDNLink component inside div is a better pattern. On the other hand, I'd prefer to wrap parentheses around outer component if expression is more than one line.

```
return (
  div({ className: "panel-container" },
    timelines,
    MDNLink({
      url: TIMINGS_MDN_URL,
    }),
  )
);
```
Comment on attachment 8853895 [details]
Bug 1324533 - link timings panel to explanation.

Cancel r? from ntim, I've told locke on Slack to adopt mozreview recommended review mechanism and modify commit message r?ntim,rchien to ask reviewer.
Attachment #8853895 - Flags: review?(rchien)
Flags: needinfo?(ntim.bugs)
Comment on attachment 8853895 [details]
Bug 1324533 - link timings panel to explanation.

https://reviewboard.mozilla.org/r/125930/#review129370

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:906
(Diff revision 7)
> -.headers-summary .learn-more-link {
> +.learn-more-link {
>    color: var(--theme-highlight-blue);
>    cursor: pointer;
>    margin: 0 5px;
>    white-space: nowrap;
>    flex-grow: 1;
>  }
>  
> -.headers-summary .learn-more-link:hover {
> +.learn-more-link:hover {

Can you move this section to line 855, just before /* Headers tabpanel */ ?
Attachment #8853895 - Flags: review?(ntim.bugs) → review+
Comment on attachment 8853895 [details]
Bug 1324533 - link timings panel to explanation.

https://reviewboard.mozilla.org/r/125930/#review129398

::: devtools/client/netmonitor/src/utils/mdn-utils.js:146
(Diff revision 7)
>  ];
>  
>  const GA_PARAMS =
>    "?utm_source=mozilla&utm_medium=devtools-netmonitor&utm_campaign=default";
>  
> +const NETWORK_MONITOR_TIMINGS_MDN_URL = "https://developer.mozilla.org/docs/Tools/Network_Monitor#Timings";

nit: Looks like this line exceeds 90 chars, it will cause eslint error.
Attachment #8853895 - Flags: review?(rchien) → review+
Thank you Locke! Everything looks great to me. Please address the nit and make sure try (our CI) is green before asking checkin-needed.

Good job!
Comment on attachment 8854771 [details]
Bug 1324533 - Moved class '.lean-more-link' to line 855.

https://reviewboard.mozilla.org/r/126756/#review129420
Attachment #8854771 - Flags: review?(ntim.bugs) → review+
Attachment #8854771 - Attachment is obsolete: true
Attachment #8854818 - Attachment is obsolete: true
Attachment #8854818 - Flags: review?(rchien)
Keywords: checkin-needed
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bb83b9787fbf
link timings panel to explanation. r=ntim,rickychien
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bb83b9787fbf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Iteration: --- → 55.3 - Apr 17
Priority: P3 → P1
Hi, I found that the CSS changes here introduces a regression.  See the attached screenshot.  The links for the headers are not right-aligned anymore.  Can it be fixed here or does it need a new bug?
Flags: needinfo?(gasolin)
I found it when working on Bug 1350233.  So perhaps I can just fix it in that bug?
(In reply to Michael Brennan from comment #37)
> I found it when working on Bug 1350233.  So perhaps I can just fix it in
> that bug?
That would be great, thanks!

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #38)
> (In reply to Michael Brennan from comment #37)
> > I found it when working on Bug 1350233.  So perhaps I can just fix it in
> > that bug?
> That would be great, thanks!
> 
> Honza

Sure! :-)
Flags: needinfo?(gasolin)
I’ve reproduced the issue described in comment 0 using old Firefox 55.0a1 (Build Id:20161219030207) and verified that the [Learn More] link is available on Firefox 55.0a1 (Build Id:20170409123541) using Mac OS X 10.12, Ubuntu 16.04 64bit and Windows 10 64bit.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: qe-verify+
See Also: → 1398709
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: