Closed Bug 1207997 Opened 5 years ago Closed 4 years ago

Consider wrapping about:debugging UI components with `createFactory`.

Categories

(DevTools :: about:debugging, defect)

defect
Not set

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: janx, Assigned: jdescottes)

References

Details

Attachments

(1 file)

(James Long (:jlongster) in bug 1196785 comment 25)
> Oh I see, you literally call `React.createElement`. It works for now, but it
> is quite verbose and I would recommend separating the ui components into
> different files in a `components` folder and wrapping them with
> `createFactory` when importing them. I don't know if we should block this on
> that or not, at least file a follow-up bug I guess.

This is the suggested follow-up bug.
Summary: Consider separating "about:debugging" UI components into different files. → Consider wrapping about:debugging UI components with `createFactory`.
Component: Developer Tools → Developer Tools: about:debugging
James, about:debugging landed, and calls `React.createElement` for all the required components.

What would be the benefit of changing this to `createFactory`? (I've never done this before, and don't have much experience with React apart from the about:debugging components.)
Flags: needinfo?(jlong)
Hey Jan, sorry for not replying earlier.

The benefit is just less verbosity. Going from:

React.createElement(
  Toolbar,
  { links: ['one', 'two', 'three'] },
  React.createElement(Button, { label: 'one' }),
  React.createElement(Button, { label: 'two' }),
  React.createElement(Button, { label: 'three' })
);

To:

Toolbar({ links: ['one', 'two', 'three'] },
        Button({ label: 'one' }),
        Button({ label: 'two' }),
        Button({ label: 'three' }))

Is a lot cleaner, and the structure is more apparent. We're trying to decide if we should use JSX, because even the latter isn't as clean as JSX. But that's a ways off if we ever do it because we'll need a build step.

I hope I'm understanding your question correctly.

Using `createFactory` turns a component class into a callable function. You should do this whenever you import a component:

const Toolbar = React.createFactory(require('toolbar'));

And then you can call it like `Toolbar()` now. It makes importing a little verbose, but it's not too bad.
Flags: needinfo?(jlong)
Note that I believe several places where we are writing React (Jordan's memory tool, and I think Honza's work) already does this so it's already pretty standard (folks contributing React will expect this too).
Alex, since you're rewriting some of about:debugging's React code, please consider James' suggestion for any new code you're adding.
Flags: needinfo?(poirot.alex)
I have some refactoring in progress, so I'll try to sneak this in.
Flags: needinfo?(poirot.alex)
Depends on: 1250002
Comment on attachment 8723584 [details]
MozReview Request: Bug 1207997 - about:debugging use factories/dom helpers instead of createElement;r=janx

https://reviewboard.mozilla.org/r/36635/#review33369

Using `createFactory` does make the code a lot more readable, thanks a lot for the patch Julian!

Mostly silly nits, but I think you forgot to change the references to `React` in `initializer.js`.

::: devtools/client/aboutdebugging/components/addons-controls.js:15
(Diff revision 1)
> -const React = require("devtools/client/shared/vendor/react");
> +const { createClass, DOM: dom} = require("devtools/client/shared/vendor/react");

Nit: Please add a space between `dom` and `}`.

::: devtools/client/aboutdebugging/components/addons-controls.js:29
(Diff revision 1)
> -    return React.createElement(
> -      "div", { className: "addons-controls" }, React.createElement(
> +    return dom.div(
> +        { className: "addons-controls" },

Nit: When there is space, please place the `props` object on the same line as the created element.

::: devtools/client/aboutdebugging/components/addons-tab.js:12
(Diff revision 1)
> -const { TabHeader } = require("./tab-header");
> -const { TargetList } = require("./target-list");
> +const TargetList = createFactory(require("./target-list"));
> +const TabHeader = createFactory(require("./tab-header"));
> +const AddonsControls = createFactory(require("./addons-controls"));

Nit: Please sort the `createFactory` lines alphabetically (`AddonsControls` comes first).

::: devtools/client/aboutdebugging/components/addons-tab.js:58
(Diff revision 1)
> -    return React.createElement(
> +    return dom.div({ id: "tab-addons", className: "tab", role: "tabpanel",
> -      "div", { id: "tab-addons", className: "tab", role: "tabpanel",
>          "aria-labelledby": "tab-addons-header-name" },

Nit: Our styleguide for `props` arguments that don't fit in a single line is inconsistent.

What about we always split them one prop per line if everything doesn't fit in one line? E.g.

dom.div({
  id: "tab-addons",
  className: "tab",
  role: "tabpanel",
  /* ... */
})

::: devtools/client/aboutdebugging/components/addons-tab.js:60
(Diff revision 1)
> -        React.createElement(TabHeader, {
> -          id: "tab-addons-header-name",
> +        TabHeader({ id: "tab-addons-header-name",
> +          name: Strings.GetStringFromName("addons") }),

Nit: Please split the `props` argument one prop per line.

::: devtools/client/aboutdebugging/components/workers-tab.js:13
(Diff revision 1)
> -const { TabHeader } = require("./tab-header");
> +const TargetList = createFactory(require("./target-list"));
> +const TabHeader = createFactory(require("./tab-header"));

Nit: Please sort the `createFactory` lines alphabetically.

::: devtools/client/aboutdebugging/components/workers-tab.js:52
(Diff revision 1)
> -    return React.createElement(
> +    return dom.div({ id: "tab-workers", className: "tab", role: "tabpanel",
> -      "div", { id: "tab-workers", className: "tab", role: "tabpanel",
>          "aria-labelledby": "tab-workers-header-name" },

Nit: Please split the `props` argument one prop per line.

::: devtools/client/aboutdebugging/initializer.js:40
(Diff revision 1)
>        let app = React.createElement(AboutDebuggingApp, { client, telemetry });
>        React.render(app, document.querySelector("#body"));

Nit: Oops, you forgot a call to `React.createElement` here.

Nit: Also, `React` is no longer imported in this file (even though it seems to exist in the global scope), so please either add it to ESLint's `/* global */` list or remove its references from this file.
Attachment #8723584 - Flags: review?(janx) → review+
The code definitely looks much cleaner now, thanks for the patch Julian!
Assignee: nobody → jdescottes
Comment on attachment 8723584 [details]
MozReview Request: Bug 1207997 - about:debugging use factories/dom helpers instead of createElement;r=janx

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36635/diff/1-2/
https://hg.mozilla.org/mozilla-central/rev/1e55f160b4ae
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.