Closed Bug 1339686 Opened 3 years ago Closed 3 years ago

Convert MDN [learn more] link as a react component

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox54 verified)

VERIFIED FIXED
Firefox 54
Iteration:
54.2 - Feb 20
Tracking Status
firefox54 --- verified

People

(Reporter: gasolin, Assigned: gasolin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(1 file)

Since there are some discussion about how to present MDN links in netmonitor, we could wrap it as a component and let the followup switch more easier.

This bug should

* create shared/components/MdnLink component
* merge shared/components/headers-mdn.js into headers-panel.js
Iteration: --- → 54.2 - Feb 20
Flags: qe-verify?
Priority: -- → P1
The patch also fixed the matching header case bug, now support header with any case (ex: etag, ETag, ETAG)
Comment on attachment 8837419 [details]
Bug 1339686 - Convert MDN [learn more] link as a react component;

https://reviewboard.mozilla.org/r/112556/#review114020

::: devtools/client/netmonitor/shared/components/headers-panel.js:23
(Diff revision 1)
>  const { REPS, MODE } = require("devtools/client/shared/components/reps/load-reps");
>  const Rep = createFactory(REPS.Rep);
>  
>  // Components
>  const PropertiesView = createFactory(require("./properties-view"));
> +const MdnLink = createFactory(require("./mdn-link"));

nit: alphabetical order

::: devtools/client/netmonitor/shared/components/headers-panel.js:25
(Diff revision 1)
>  
>  // Components
>  const PropertiesView = createFactory(require("./properties-view"));
> +const MdnLink = createFactory(require("./mdn-link"));
>  
> -const { a, div, input, textarea } = DOM;
> +const { div, input, textarea } = DOM;

nit: add a new line after const { div, input, textarea } = DOM;

::: devtools/client/netmonitor/shared/components/headers-panel.js:45
(Diff revision 1)
>  
> +/**
> + * A mapping of header names to external documentation. Any header included
> + * here will show a MDN link alongside it.
> + */
> +var URL_DOMAIN = "https://developer.mozilla.org";

nit: const

::: devtools/client/netmonitor/shared/components/headers-panel.js:45
(Diff revision 1)
> +var URL_DOMAIN = "https://developer.mozilla.org";
> +const URL_PATH = "/en-US/docs/Web/HTTP/Headers/";
> +const URL_PARAMS =
> +  "?utm_source=mozilla&utm_medium=devtools-netmonitor&utm_campaign=default";

I think we can merge these URL variables since they are seperated but it's not useful in different purposes.

We can do one template string with a dynamic header like this:

`https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/${headers}?utm_source=mozilla&utm_medium=devtools-netmonitor&utm_campaign=default`

::: devtools/client/netmonitor/shared/components/headers-panel.js:50
(Diff revision 1)
> +var URL_DOMAIN = "https://developer.mozilla.org";
> +const URL_PATH = "/en-US/docs/Web/HTTP/Headers/";
> +const URL_PARAMS =
> +  "?utm_source=mozilla&utm_medium=devtools-netmonitor&utm_campaign=default";
> +
> +var SUPPORTED_HEADERS = [

nit: const

::: devtools/client/netmonitor/shared/components/headers-panel.js:303
(Diff revision 1)
> -
> -  let win = Services.wm.getMostRecentWindow(gDevTools.chromeWindowType);
> -  win.openUILinkIn(headerDocURL, "tab");
> -}
> -
>  function renderValue(props) {

nit: 

move this component functino into createClass object since we're using createClass here. Remember put it in right order.

::: devtools/client/netmonitor/shared/components/headers-panel.js:337
(Diff revision 1)
> + * The baseURL to use.
> + *
> + * @return {string}
> + * The MDN URL for the header, or null if not available.
> + */
> +function getURL(header) {

nit: likewise above

::: devtools/client/netmonitor/shared/components/mdn-link.js:7
(Diff revision 1)
> +const {
> +  DOM,
> +  PropTypes,
> +} = require("devtools/client/shared/vendor/react");
> +const { L10N } = require("../../l10n");
> +const Services = require("Services");
> +const { gDevTools } = require("devtools/client/framework/devtools");

nit:

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

::: devtools/client/netmonitor/shared/components/mdn-link.js:19
(Diff revision 1)
> +
> +const { a } = DOM;
> +
> +const LEARN_MORE = L10N.getStr("netmonitor.headers.learnMore");
> +
> +function onLearnMoreClick(e, link) {

nit: move this function onLearnMoreClick() after MdnLink.propTypes = ...

::: devtools/client/netmonitor/shared/components/mdn-link.js:27
(Diff revision 1)
> +
> +  let win = Services.wm.getMostRecentWindow(gDevTools.chromeWindowType);
> +  win.openUILinkIn(link, "tab");
> +}
> +
> +function MdnLink({ link }) {

nit: I prefer MDNLink

::: devtools/client/netmonitor/shared/components/mdn-link.js:40
(Diff revision 1)
> +}
> +
> +MdnLink.displayName = "MdnLink";
> +
> +MdnLink.propTypes = {
> +  link: PropTypes.string,

nit: isRequired.
I was about to file this as a separate bug, but as the case-sensitivity is fixed here as well, I want to bring up that the URL "https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/" should be without "en-US". 

By using https://developer.mozilla.org/docs/Web/HTTP/Headers/, MDN will redirect to the preferred language of the user. If the page isn't translated yet, like currently https://developer.mozilla.org/fr/docs/Web/HTTP/Headers/Cache-Control, there will be the English content and a call to action to contribute a translation to MDN. The JS error [Learn more] links in the web console use the same language-agnostic URL and it has been very helpful to get the documentation translated.
Florian thanks for comment. I removed `en-US` in url.


The `getURL` function is also used in test, therefore I put it in a separate file utils/mdn-utils.js
Comment on attachment 8837419 [details]
Bug 1339686 - Convert MDN [learn more] link as a react component;

https://reviewboard.mozilla.org/r/112556/#review114078

Thanks for working on this. I like the way to extract a separate mdn-utils.js for common purpose. It's also helpful while testing.

::: devtools/client/netmonitor/utils/mdn-utils.js:85
(Diff revision 4)
> + *
> + * @param {string} header Name of the header for the baseURL to use.
> + *
> + * @return {string} The MDN URL for the header, or null if not available.
> + */
> +function getURL(header) {

nit: getHeadersURL

There might be other URL rather header in the future.
Attachment #8837419 - Flags: review+
Blocks: 1337690
Depends on: 1337737
Comment on attachment 8837419 [details]
Bug 1339686 - Convert MDN [learn more] link as a react component;

https://reviewboard.mozilla.org/r/112556/#review114562

LGTM!

Honza
Attachment #8837419 - Flags: review?(odvarko) → review+
Keywords: checkin-needed
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e1b7c9e91404
Convert MDN [learn more] link as a react component;r=Honza,rickychien
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e1b7c9e91404
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Flags: qe-verify? → qe-verify+
QA Contact: ciprian.georgiu
I've managed to verify this bug using latest Nightly 54.0a1 (2017-03-06) on Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 16.04 x86 LTS. I can confirm that MDN links in netmonitor works as expected.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.