1311591 - Implement clear button for Net Panel Toolbar

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Ricky Chien [:rickychien] (inactive)]

Follow-up for bug 1309191 to focus on implementing clear button.

Comment 1

[[e-mail]]

Attachment: Bug 1311591 - Implement clear button for Net Panel Toolbar;

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

Comment 2

[[e-mail]]

Comment on attachment 8803772 [details]
Bug 1311591 - Implement clear button for Net Panel Toolbar;

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

Comment 3

[[e-mail]]

The WIP converted clear button to react, the next patch will use `CLEAR_REQUESTS` action and the `requests` reducer to replace the requestsMenu.clear method.

Comment 4

[[e-mail]]

Attachment: Bug 1311591 - Convert RequestsMenu.clear to redux action/reducer;

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

Comment 5

[[e-mail]]

Comment on attachment 8803772 [details]
Bug 1311591 - Implement clear button for Net Panel Toolbar;

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

Comment 6

[[e-mail]]

Comment on attachment 8803779 [details]
Bug 1311591 - Convert RequestsMenu.clear to redux action/reducer;

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

Comment 7

[[e-mail]]

Comment on attachment 8803779 [details]
Bug 1311591 - Convert RequestsMenu.clear to redux action/reducer;

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

Comment 8

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8803772 [details]
Bug 1311591 - Implement clear button for Net Panel Toolbar;

https://reviewboard.mozilla.org/r/87918/#review86924

::: devtools/client/netmonitor/toolbar-view.js:33
(Diff revision 3)
>  
> +    this._clearContainerNode = $("#react-clear-button-hook");
>      this._filterContainerNode = $("#react-filter-buttons-hook");
>  
> +    // clear button
> +    ReactDOM.render(button({

Unmount this component in destroy().

::: devtools/client/netmonitor/toolbar-view.js:39
(Diff revision 3)
> +      ref: (node) => {
> +        node && node.setAttribute("tooltiptext",
> +          L10N.getStr("netmonitor.toolbar.clear"));
> +      }

Replace this with a simple "title" attribute for the <button> element.

::: devtools/client/themes/toolbars.css:55
(Diff revision 3)
>    padding: 0;
>    border-width: 0;
>    border-bottom-width: 1px;
>    border-style: solid;
>    height: 24px;
> -  line-height: 24px;
> +  line-height: -moz-block-height;

What does this style change do? It's a global CSS rule that affects all devtools-toolbar/devtools-sidebar-tabs components, so it can potentially break things outside Netmonitor.

Comment 9

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8803779 [details]
Bug 1311591 - Convert RequestsMenu.clear to redux action/reducer;

https://reviewboard.mozilla.org/r/87936/#review86928

::: devtools/client/netmonitor/reducers/requests.js:14
(Diff revision 3)
> +/**
> + * Removes all network requests and closes the sidebar if open.
> + */
> +function clearRequests() {
> +  NetMonitorController.NetworkEventsHandler.clearMarkers();
> +  NetMonitorView.Sidebar.toggle(false);
> +
> +  $("#details-pane-toggle").disabled = true;
> +  $("#requests-menu-empty-notice").hidden = false;
> +
> +  NetMonitorView.RequestsMenu.empty();
> +  NetMonitorView.RequestsMenu.refreshSummary();
> +  return new Requests();
> +}

No, this is wrong. Reducer must be a pure function without side effects. It's only allowed to transform the state and return a new one.

Side effects like toggling a sidebar or emptying the request list must be handled by subscribing to the store updates.

I think that introducing a redux action in this bug is not a good idea - it just introduces a lot of boilerplate code that doesn't do anything. There is no state to store - the "Requests" field is a dummy noop - and therefore no state changes to react to.

Let's postpone implementing the "clear" action to bug 1309866. That bug introduces a real requests list into the Redux store.

Comment 10

[[e-mail]]

Comment on attachment 8803772 [details]
Bug 1311591 - Implement clear button for Net Panel Toolbar;

https://reviewboard.mozilla.org/r/87918/#review86924

> Unmount this component in destroy().

nice catch!

> Replace this with a simple "title" attribute for the <button> element.

good to know that

> What does this style change do? It's a global CSS rule that affects all devtools-toolbar/devtools-sidebar-tabs components, so it can potentially break things outside Netmonitor.

I'll replace this fix to a separate #react-clear-button-hook style in netmonitor.css

Comment 11

[[e-mail]]

Comment on attachment 8803772 [details]
Bug 1311591 - Implement clear button for Net Panel Toolbar;

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

Comment 12

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8803772 [details]
Bug 1311591 - Implement clear button for Net Panel Toolbar;

https://reviewboard.mozilla.org/r/87918/#review87046

::: devtools/client/themes/netmonitor.css:10
(Diff revision 4)
> +#react-clear-button-hook {
> +  line-height: -moz-block-height;
> +}

Looks good, but this style change is still worth explaining.

Comment 13

[Jarda Snajdr [:jsnajdr]]

(In reply to Jarda Snajdr [:jsnajdr] from comment #12)
> > +#react-clear-button-hook {
> > +  line-height: -moz-block-height;
> > +}
> 
> Looks good, but this style change is still worth explaining.

Hmmm, there is a mysterious 3px whitespace above the clear button, I have no idea where it comes from.

Setting display:flex on the #react-clear-button-hook element is a better fix than line-height - the clear button is vertically centered then.

Comment 14

[[e-mail]]

Comment on attachment 8803772 [details]
Bug 1311591 - Implement clear button for Net Panel Toolbar;

https://reviewboard.mozilla.org/r/87918/#review87046

> Looks good, but this style change is still worth explaining.

The style change is to fix the extra hight when we add extra html:div as react component's hook point

Comment 15

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8803772 [details]
Bug 1311591 - Implement clear button for Net Panel Toolbar;

https://reviewboard.mozilla.org/r/87918/#review87278

r+ if the CSS rule for centering the clear button gets fixed and if try is green.

Comment 16

[[e-mail]]

Comment on attachment 8803772 [details]
Bug 1311591 - Implement clear button for Net Panel Toolbar;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87918/diff/4-5/

Comment 17

[[e-mail]]

`display:flex` works well, thanks for suggestion!

try is green

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc6654e17667
Implement clear button for Net Panel Toolbar;r=jsnajdr

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/fc6654e17667

Comment 20

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm the clear button works accordingly. I verified using Fx 52.0a1 (20161108030212).