Brian Grinstead [:bgrins] Reporter
	Description • 9 years ago

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.

	Tim Nguyen :ntim Assignee
	Comment 2 • 8 years ago

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

	Gabriel [:gl] (ΦωΦ)
	Comment 3 • 8 years ago

This looks really good! Thanks for the patch ntim!

	Jarda Snajdr [:jsnajdr]
	Comment 4 • 8 years ago
mozreview-review

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.

	Pulsebot
	Comment 7 • 8 years ago

Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5782449527ee
Create shared React component for searchbox. r=jsnajdr

	Tim Nguyen :ntim Assignee
	Comment 8 • 8 years ago

Thanks for the review! Addressed your comments.

	Pulsebot
	Comment 9 • 8 years ago

Pushed by archaeopteryx@coole-files.de:
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

	Carsten Book [:Tomcat]
	Comment 10 • 8 years ago
bugherder

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