1154789 - Create searchbox React component for devtools

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P1)

Description

[Brian Grinstead [:bgrins]]

To get 'filter rules' (bug 1120616) landed, we have some code duplication between the rule view and computed view.  Specifically, there is a new element with markup like this:

<div class="devtools-searchbox">
  <input id="ruleview-searchbox"
         class="devtools-searchinput devtools-rule-searchbox"
         type="search" placeholder="&userStylesSearch;"/>
  <button id="ruleview-searchinput-clear" class="devtools-searchinput-clear"></button>
</div>

It handles events for clearing the input when the devtools-searchinput-clear button is clicked, when to show and hide that button, a timeout before firing a change event, and context menu integration with the toolbox-textbox-context-popup element.

This would be generally useful for other tools, and also would allow us to clean up some duplicated code throughout the rule view and computed view.  We can also have a single test in place for that widget that would replace both browser_computedview_search-filter_context-menu.js and browser_ruleview_search-filter_context-menu.js.

Comment 1

[Tim Nguyen :ntim]

Attachment: Bug 1154789 - Create shared React component for searchbox.

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

Comment 2

[Tim Nguyen :ntim]

The component isn't being integrated anywhere yet, so no QA needed

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

This looks really good! Thanks for the patch ntim!

Comment 4

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8800204 [details]
Bug 1154789 - Create shared React component for searchbox.

https://reviewboard.mozilla.org/r/85200/#review83804

Looks good, except few nits about style and performance.

::: devtools/client/shared/components/search-box.js:13
(Diff revision 1)
> +const {KeyShortcuts} = require("devtools/client/shared/key-shortcuts");
> +
> +/**
> + * Generic list component that takes another react component to represent
> + * the children nodes as `itemComponent`, and a list of items to render
> + * as that component with a click handler.

The comment should describe this component, not another one.

::: devtools/client/shared/components/search-box.js:31
(Diff revision 1)
> +      value: ""
> +    };
> +  },
> +
> +  componentDidMount() {
> +    this.setState({ value: this.refs.input.value });

Nit: check that the value is really different before calling setState. The call to setState will trigger another render, which is almost never needed.

::: devtools/client/shared/components/search-box.js:38
(Diff revision 1)
> +    if (!this.props.keyShortcut) {
> +      return;
> +    }
> +
> +    let shortcuts = this.shortcuts = new KeyShortcuts({
> +      window

style nit: why creating a local variable for "shortcuts"?

::: devtools/client/shared/components/search-box.js:92
(Diff revision 1)
> +      inputClassList.push("filled");
> +    }
> +    return dom.div(
> +      { className: divClassList.join(" ") },
> +      dom.input(Object.assign({}, this.props, {
> +        className: inputClassList.join(" "),

Nit: it seems like "type" is the only prop that makes sense to pass as a property to the dom.input. Then why not simply:

dom.input({
  type: ...
  className: ...
  ...
});

Or do you want to support passing other properties like "style"?

::: devtools/client/shared/components/search-box.js:101
(Diff revision 1)
> +      dom.button({
> +        className: "devtools-searchinput-clear",
> +        onClick: this.onClearButtonClick,
> +        hidden: value == "",
> +        ref: "clear-button"
> +      })

Nit: the clear-button ref is not needed anywhere.

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8800204 [details]
Bug 1154789 - Create shared React component for searchbox.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85200/diff/1-2/

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8800204 [details]
Bug 1154789 - Create shared React component for searchbox.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85200/diff/2-3/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5782449527ee
Create shared React component for searchbox. r=jsnajdr

Comment 8

[Tim Nguyen :ntim]

Thanks for the review! Addressed your comments.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e3a490dbfea7
Create shared React component for searchbox: tell eslint that search-box.js has windows as global. r=eslint-fix

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5782449527ee
https://hg.mozilla.org/mozilla-central/rev/e3a490dbfea7