1350233 - Add [learn more] MDN link for statistics panel

Status: VERIFIED FIXED

(DevTools :: Netmonitor, enhancement, P1)

Description

[[e-mail]]

We'd like to have a [Learn More] link to MDN on statistics panel, check
https://developer.mozilla.org/docs/Tools/Network_Monitor#Performance_analysis

Refer Bug 1323454 - Network panel: integrate HTTP Status code with MDN and Bug 1339686 - Convert MDN [learn more] link as a react component

Comment 1

[[e-mail]]

You need Patch statistics-panel.js
http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/components/statistics-panel.js

and add an MDNLink component at the end of 
http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/shared/components/mdn-link.js

Before set it as a good-first-bug, we need find a good place to put the Learn More link in statistics panel...
Blake, any suggestion?

Comment 2

[Blake Winton (:bwinton) (:☕️)]

(In reply to Fred Lin [:gasolin] from comment #0)
> We'd like to have a [Learn More] link to MDN on statistics panel, check
> https://developer.mozilla.org/docs/Tools/Network_Monitor#Performance_analysis

I'm not sure we do.  Which MDN url would it lead to?

Comment 3

[[e-mail]]

The UI has been discussed between developers (Honza, me, and Ricky) last week, the rational is to help user understand what statistics panel does. So we try to do some simple thing to help user know more about the statistics view.

The link would lead to
https://developer.mozilla.org/docs/Tools/Network_Monitor#Performance_analysis


There seems no obvious place in the statistics panel, maybe add the link in the empty list view?

```
• Click on the O button to start performance analysis. [Learn More]
```

Comment 4

[Blake Winton (:bwinton) (:☕️)]

So, with no better place to put it, we might as well stick it at the bottom center until the panel gets redesigned…

Comment 5

[Michael Hoffmann]

Should the link be placed in the empty request list view or in the statistics panel?  Or perhaps in both?

If a link is placed in the statistics panel at the bottom center it will be right below the vertical splitter, which does not look so nice as the splitter cannot extend all the way to the bottom anymore.  I'm attaching a screenshot.  Perhaps some clever styling could make that look less bad.

What about putting it on the lower right corner instead, stealing some area from the empty cache chart?  And probably lower left for RTL locales.

Comment 6

[Michael Hoffmann]

Attachment: statistics-panel.png

This screenshot suffers from the overflow problem described in Bug 1339558

Comment 7

[Michael Hoffmann]

Attachment: request-list.png

Comment 8

[Blake Winton (:bwinton) (:☕️)]

(In reply to Michael Brennan from comment #5)
> Should the link be placed in the empty request list view or in the
> statistics panel?  Or perhaps in both?

Both seem reasonable.  People often like to know what they're going to get before they click on it.

> What about putting it on the lower right corner instead, stealing some area
> from the empty cache chart?  And probably lower left for RTL locales.

WFM.

Comment 9

[[e-mail]]

Thanks Michael, as Blake said, we can put [Learn More] in both place, and I also prefer put the [Learn More] link on the lower right corner in Statistics View. 

Please also check if it shows correctly when we put the whole devtools panel in the right-side of the browser.

Comment 10

[[e-mail]]

While deal with URL in `utils/mdn-utils`, it will be nice to distill all `https://developer.mozilla.org/docs/` as `MDN_URL` and construct links as `${MDN_URL}Tools/Network_Monitor#Performance_analysis${GA_PARAMS}`

Comment 11

[Michael Hoffmann]

Attachment: bug1350233.patch

Attaching a patch for feedback.

To keep things simple I've just placed the link in the statistics panel with `position: absolute`.  This can make the link overlap with other text if the panels are resized to be too small, but I'm thinking it might not be a problem.  What do you think?

The reason for changing `.notice-perf-message` to use `display: flex` is to make the link appear to the left of the text in RTL locales.  Don't know if that's the best way to achieve that, but it did make it work.

Comment 12

[[e-mail]]

Comment on attachment 8855496 [details] [diff] [review]
bug1350233.patch

Thanks for the patch!

I've talked with Honza and he suggest put the [Learn More] link near the title of each pie chart(after `Primed cache` & `Empty cache`). Then we don't have change pie chart css to find the room for [Learn More]. We can change [Learn More] to different links later when we have better document for each chart.


```
function getNetMonitorTimingsURL() {
  return NETWORK_MONITOR_TIMINGS_MDN_URL;
}
```

We can remove `NETWORK_MONITOR_TIMINGS_MDN_URL` and use the similar pattern to construct the URL.

Comment 13

[Michael Hoffmann]

Thanks for the feedback!

(In reply to Fred Lin [:gasolin] from comment #12)

> I've talked with Honza and he suggest put the [Learn More] link near the
> title of each pie chart(after `Primed cache` & `Empty cache`). Then we don't
> have change pie chart css to find the room for [Learn More]. We can change
> [Learn More] to different links later when we have better document for each
> chart.

Yes, sounds like a good idea.  I've implemented it in the patch I'm about to attach.  It wasn't that straightforward as Chart isn't a React component.  We'll see if my solution is good enough.

> 
> ```
> function getNetMonitorTimingsURL() {
>   return NETWORK_MONITOR_TIMINGS_MDN_URL;
> }
> ```
> 
> We can remove `NETWORK_MONITOR_TIMINGS_MDN_URL` and use the similar pattern
> to construct the URL.

Thanks!  That one appeared after I did a rebase so I missed it.

Comment 14

[Michael Hoffmann]

Attachment: bug1350233.patch

This also fixes the problem mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1324533#c36

Comment 15

[Michael Hoffmann]

I forgot to remove the link in statistics-panel.js:295: https://bugzilla.mozilla.org/attachment.cgi?id=8855770&action=diff#a/devtools/client/netmonitor/src/components/statistics-panel.js_sec6
It should of course be removed.

Comment 16

[[e-mail]]

Comment on attachment 8855770 [details] [diff] [review]
bug1350233.patch

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

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css
@@ +155,5 @@
>  
>  .notice-perf-message {
>    margin-top: 2px;
> +  display: flex;
> +  align-items: center;

Look Nice!

@@ +1161,5 @@
> +  font-weight: 400;
> +}
> +
> +.statistics-panel .table-chart-title {
> +  display: flex;

It seems works fine without this style, do we really need this?

::: devtools/client/netmonitor/src/components/request-list-empty-notice.js
@@ +14,5 @@
>  const Actions = require("../actions/index");
>  const { ACTIVITY_TYPE } = require("../constants");
>  const { L10N } = require("../utils/l10n");
> +const { getPerformanceAnalysisURL } = require("../utils/mdn-utils");
> +const MDNLink = createFactory(require("./mdn-link"));

To keep consistency of declaration (ex network-details-panel.js), we can put related declaration like:

```
const { getPerformanceAnalysisURL } = require("../utils/mdn-utils");

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

const { button, div, span } = DOM;
```

::: devtools/client/netmonitor/src/components/statistics-panel.js
@@ +25,5 @@
>  const { button, div } = DOM;
>  const MediaQueryList = window.matchMedia("(min-width: 700px)");
>  
> +const { getPerformanceAnalysisURL } = require("../utils/mdn-utils");
> +const MDNLink = createFactory(require("./mdn-link"));

same here,

```
const { L10N } = require("../utils/l10n");
const { getPerformanceAnalysisURL } = require("../utils/mdn-utils");

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

const { button, div } = DOM;
```

Comment 17

[Blake Winton (:bwinton) (:☕️)]

(In reply to Michael Brennan from comment #14)
> Created attachment 8855770 [details] [diff] [review]
> bug1350233.patch

Since this is user-facing code, you'll also going to need a ui-review from me.  Please add that flag to the patch when you feel it's ready for review.

Thanks!  :)

Comment 18

[Michael Hoffmann]

(In reply to Fred Lin [:gasolin] from comment #16)
> @@ +1161,5 @@
> > +  font-weight: 400;
> > +}
> > +
> > +.statistics-panel .table-chart-title {
> > +  display: flex;
> 
> It seems works fine without this style, do we really need this?
> 

It makes the link appear to the left of the title in RTL-locales, at least when I try with force RTL.  Please let me know if this is the wrong way to accomplish this.

Comment 19

[Michael Hoffmann]

Attachment: bug1350233.patch

Comment 20

[[e-mail]]

Comment on attachment 8856734 [details] [diff] [review]
bug1350233.patch

Looks good to me, thanks!

Comment 21

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8856734 [details] [diff] [review]
bug1350233.patch

It's a little weird to have two links pointing to the same place, but hopefully that'll be resolved sometime soon.  :)  ui-r=me.

Comment 22

[[e-mail]]

Attachment: Bug 1350233 - Add [learn more] MDN link for statistics panel

Review commit: https://reviewboard.mozilla.org/r/129324/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129324/

Comment 23

[[e-mail]]

Comment on attachment 8857340 [details]
Bug 1350233 - Add [learn more] MDN link for statistics panel

test green

Comment 24

[[e-mail]]

thanks!

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing

Comment 26

[[e-mail]]

Michael, could you help rebase the patch with the latest branch?

Comment 27

[Michael Hoffmann]

Attachment: bug1350233.patch

Of course!

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f20490ec8afe
Add MDN links for netmonitor performance analysis. r=gasolin ui-r=bwinton

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 30

[Ciprian Georgiu, Desktop QA]

Reproduced the issue with Nightly from 2017-04-10.

This is verified fixed on latest Nightly 55.0a1 (2017-04-18) across platforms: Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 16.04 x64 LTS. The [Learn More] link is now displayed on statistics panel.

Comment 31

[Matthew N. [:MattN]]

Screenshots of the change:
https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=3148ecea537e093ec6c8d51527cb4045df8ee275&newProject=mozilla-central&newRev=05c212a94183838f12feebb2c3fd483a6eec18c2&filter=netmon

Note that the learn more link in the list view shifted the baseline of the existing text.

Comment 32

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

@Michael: please see comment #31. Maybe it's simple to fix this?

Honza

Comment 33

[Michael Hoffmann]

(In reply to Jan Honza Odvarko [:Honza] from comment #32)
> @Michael: please see comment #31. Maybe it's simple to fix this?
> 
> Honza

Hi,

I tried with `align-items: baseline` which moved the line a small amount, but it is still different from its original position.  I'm not too good with CSS and I don't know how to ensure that the original position is kept while using `display: flex`.  Not changing to flex solves this issue, but then the link will still appear to the right of the text with Force RTL.

So I don't know what the best way to solve this is.  Is there perhaps a better way to make the link appear correctly in RTL locales without using flexbox?

Comment 34

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

@Tim, any tips? (read from comment #31)

Honza

Comment 35

[Tim Nguyen :ntim]

I don't think it's a major issue IMO. The alignment actually looks improved to me.

Comment 36

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

(In reply to Tim Nguyen :ntim from comment #35)
> I don't think it's a major issue IMO. The alignment actually looks improved
> to me.
okay, works for me.

Honza

Comment 37

[Michael Hoffmann]

Attachment: notice-perf-message.png

This is how it currently looks when the Toolbox is docked to the right

Comment 38

[Michael Hoffmann]

Attachment: notice-perf-message-better.png

This is how it probably should look when the Toolbox is docked to the right

Comment 39

[Michael Hoffmann]

By chance I just noticed that when the Toolbox is docked to the right, things does not look so good.  Please see the attached screenshots above.  This is because of Flexbox and it's probably best to just get rid of it.  Then it will look like in the second screenshot.

As for RTL, I don't know how to solve it.  I tried to conditionally put the link either before or after the line with javascript, but I still couldn't get it to work as there is some magic happening automatically with the full stop and the bullet point when I use Force RTL.  I don't speak any RTL languages and perhaps I'm overthinking this.

Should I attach a patch that makes this look fine for English, and then leave the RTL-problem for a new bug where someone who knows this can investigate?  If it even is a problem, that is.

Comment 40

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

Attachment: sidebar.png

(In reply to Michael Brennan from comment #39)
> By chance I just noticed that when the Toolbox is docked to the right,
> things does not look so good.  Please see the attached screenshots above. 
> This is because of Flexbox and it's probably best to just get rid of it. 
> Then it will look like in the second screenshot.
I can't see this issue on my machine (Win10), see the attached screenshot (btw. the side bar can't be thinner).

In any case, please file a new bug report and we can discuss how to fix the problem. This bug is already closed.

Thanks!
Honza

Comment 41

[Michael Hoffmann]

(In reply to Jan Honza Odvarko [:Honza] from comment #40)
> In any case, please file a new bug report and we can discuss how to fix the
> problem. This bug is already closed.

Sure! I've filed Bug 1358814