1302226 - Convert Storage inspector toolbar to HTML

Status: NEW

(DevTools :: Storage Inspector, enhancement, P2)

Comment 1

[Tim Nguyen :ntim]

Attachment: Patch (WIP)

Still need to fix the test.

Comment 2

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8794141 [details] [diff] [review]
Patch (WIP)

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

::: devtools/client/shared/components/search-box.js
@@ +33,5 @@
> +    });
> +    shortcuts.on(this.props.keyShortcut, (name, event) => {
> +      event.preventDefault();
> +      this.refs.input.focus();
> +    });

The KeyShortcuts object adds a 'keydown' event listener to the window. You need to call shortcuts.destroy() in componentWillUnmount. Otherwise, the listener will leak.

@@ +50,5 @@
> +    this.refs["clear-button"].hidden = !hasValue;
> +    if (hasValue && this.props.hasNoMatch) {
> +      this.refs.input.classList.add("devtools-style-searchbox-no-match");
> +    }
> +  },

You cannot manipulate the DOM elements that are rendered and owned by React. The changes will be overwritten next time React calls "render" and updates the DOM, leading to random bugs.

If the "input" or "clear-button" element has a "filled" class or is hidden or whatever, they must be set on the elements returned from the render() method. The render() method will derive the desired markup from the props and state of the component.

In this case, the SearchBox component should have a state with property "hasValue", which can be changed by calling setState().

@@ +53,5 @@
> +    }
> +  },
> +
> +  onChange() {
> +    this.updateSearchBoxState();

Replace this with:
this.setState({ hasValue: this.refs.input.value.length > 0 });

The rest of the updateSearchBoxState() method will move to render().

@@ +75,5 @@
> +  },
> +
> +  onClearButtonClick() {
> +    this.refs.input.value = "";
> +    this.onChange();

When you change the input.value from a script like this, doesn't the input fire a "change" event? Explicit call to this.onChange() is then not necessary.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8794141 [details] [diff] [review]
Patch (WIP)

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

::: devtools/client/shared/components/search-box.js
@@ +75,5 @@
> +  },
> +
> +  onClearButtonClick() {
> +    this.refs.input.value = "";
> +    this.onChange();

@jsnajdr: Nope... change events are not triggered when a value is changed programmatically.

Comment 4

[Tim Nguyen :ntim]

Attachment: WIP patch

Comment 5

[Tim Nguyen :ntim]

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #4)
> Created attachment 8800347 [details] [diff] [review]
> WIP patch

Stuff that needs to be done:
- Fix the tests
- Fix the handling of changing hosts

Comment 6

[Tim Nguyen :ntim]

The shared search box has been added in bug 1154789 at devtools/client/shared/components/search-box.js