1308426 - Implement toolbar for netmonitor panel

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Steve Chung [:steveck]]

We might want to reuse an existing components from shared directory (WebConsole/Inspector panels are using them already).

Can be split into more tasks:
- Toolbar buttons (clear and filter buttons)
- Search filter
- Summary info (including the button for opening Analysis perspective)
- Sidebar toggle button

Comment 1

[Marco Mucci [:MarcoM]]

Hi Ricky, this bug was turned into a meta.

Comment 2

[Ricky Chien [:rickychien] (inactive)]

Marco, I'd like to change my mind since this bug is going to deal with final piece implementation - converting the rest of elements to react and also converting Toolbar itself.

Is it supposed to remain as a meta or remove it from keyword?

Comment 3

[Marco Mucci [:MarcoM]]

(In reply to Ricky Chien [:rickychien] from comment #2)
> Marco, I'd like to change my mind since this bug is going to deal with final
> piece implementation - converting the rest of elements to react and also
> converting Toolbar itself.
> 
> Is it supposed to remain as a meta or remove it from keyword?

Thanks for the update Ricky.  I'll remove the meta tagging and add this work to the backlog.

Comment 4

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1308426 - Implement netmonitor toolbar

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

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Changed the title since IMO toolbar doesn't belong to shared component.

My current implementation has layout issue which appears when shrinking window size. It caused underneath RequestList header text broken. I don't know if it's due to flex layout or maybe I ran into a bug when combining HTML flex elements with XUL elements, perhaps it could be solved once RequestList is landed.

Honza, Jarda: I'd like to post my patch first to let you know that layout issue of Toolbar and RequestList. Feel free to take a look and provide solution if you have fought with it before :)

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8811789 [details]
Bug 1308426 - Implement netmonitor toolbar

https://reviewboard.mozilla.org/r/93742/#review93860

Nice!

Please see my comments inline.

Also, the search box doesn't seem to be vertically centered in the Toolbar.

Honza

::: devtools/client/netmonitor/components/clear-button.js:14
(Diff revision 1)
> +const { DOM } = require("devtools/client/shared/vendor/react");
> +const { L10N } = require("../l10n");
> +
> +const { button } = DOM;
> +
> +function ClearButton() {

An intro comment for the component would be nice.

::: devtools/client/netmonitor/components/toolbar.js:20
(Diff revision 1)
> +const SummaryButton = createFactory(require("./summary-button"));
> +const ToggleButton = createFactory(require("./toggle-button"));
> +
> +const { span } = DOM;
> +
> +function Toolbar() {

An introductionary comment for the component would be nice

::: devtools/client/netmonitor/components/toolbar.js:34
(Diff revision 1)
> +      ToggleButton()
> +    )
> +  );
> +}
> +
> +module.exports = connect()(Toolbar);

Why is the Toolbar connected to the store?

::: devtools/client/netmonitor/toolbar-view.js:18
(Diff revision 1)
>    dumpn("ToolbarView was instantiated");
>  }
>  
>  ToolbarView.prototype = {
>    /**
>     * Initialization function, called when the debugger is started.

Please update the comment (the toolbar is not for the debugger)

::: devtools/client/netmonitor/toolbar-view.js:32
(Diff revision 1)
> -      ToggleButton()
> -    ), this._toggleContainerNode);
> +      Toolbar()
> +    ), this._toolbarNode);
>    },
>  
>    /**
>     * Destruction function, called when the debugger is closed.

Please update the comment (the toolbar is not for the debugger)

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: sidebar.png

One more thing, label for side-panel section-header is now aligned to the right, see the attached screenshot.

Honza

Comment 8

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8811789 [details]
Bug 1308426 - Implement netmonitor toolbar

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

Comment 9

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8811789 [details]
Bug 1308426 - Implement netmonitor toolbar

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

Comment 10

[Ricky Chien [:rickychien] (inactive)]

Please ignore the issue on comment 5 which also appeared in https://bug1308426.bmoattachments.org/attachment.cgi?id=8811825. 

See the top left corner of the picture, "Type" header was placed in wrong position due to an incorrect flex layout in .devtools-toolbar rule.

Problem has been fixed and review issues are addressed as well. Please review it again. thanks!

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8811789 [details]
Bug 1308426 - Implement netmonitor toolbar

https://reviewboard.mozilla.org/r/93742/#review94110

Looks great!


R+, but there is one nit: if you make the browser window width small, 'clear' and 'perf-stats' icons jump one pixel up and height of the toolbar is one pixel smaller. I think it's worth fixing too.

Thanks Ricky!
Honza

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Jan Honza Odvarko [:Honza] from comment #11)
> if you make the browser window width small,
> 'clear' and 'perf-stats' icons jump one pixel up and height of the toolbar
> is one pixel smaller. I think it's worth fixing too.
My bad, this issue seems to be fixed by new version of the patch.

I think it's ready to land!

Honza

Comment 13

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8811789 [details]
Bug 1308426 - Implement netmonitor toolbar

https://reviewboard.mozilla.org/r/93742/#review94166

The patch looks good, I only think that the CSS could be even better, see below.

I didn't check the layout issue and Honza tells me it's not there any more, so I trust him :)

::: devtools/client/themes/netmonitor.css:15
(Diff revision 3)
> +.devtools-toolbar-group {
> +  display: flex;
> +  flex: 0 0 auto;
> +  flex-wrap: nowrap;
> +  align-items: center;
>  }

The ".devtools-toolbar-group" class name suggests that it's some general style intended to use across all devtools panels. In this case, it should be in one of the common styles, not in netmonitor.css.

The same comment applies for ".devtools-toolbar-container"

::: devtools/client/themes/netmonitor.css:146
(Diff revision 3)
> +#requests-menu-filter-buttons {
> +  display: flex;
> +  flex-wrap: nowrap;
> +}

There is bug 1316291 filed to rename all the "requests-menu" CSS classes to something that makes more sense.

How hard would it be to clean up the toolbar CSS rules as part of this bug?

Also, using classes instead of element IDs would be nice.

With these thankless and boring tasks, it's better to do them in smaller increments rather than to postpone to one large bug.

Comment 14

[Ricky Chien [:rickychien] (inactive)]

I agree that clean up the toolbar CSS rules as part of this bug. However, I gave up since I think it will involve more works due to the competition of css rules priorities (specificity). If we drop #IDs to write classes instead, rule might be overwritten by common.css since the rules in common.css are always longer than the new names we write. So, you have to overwrite them by longer names and it seems stupid to me.

I'd like to address this issue in bug 1316291 and figure out a better solution for it.

Comment 15

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8811789 [details]
Bug 1308426 - Implement netmonitor toolbar

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

Comment 16

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8811789 [details]
Bug 1308426 - Implement netmonitor toolbar

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b36b0dde3499
Implement netmonitor toolbar r=Honza,jsnajdr

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Ciprian Georgiu, Desktop QA]

I've managed to perform tests around Toolbar buttons, Search filter, Summary info and Sidebar toggle button across platforms:
- Windows 10 x64
- Mac OS X 10.11.5
- Ubuntu 16.04 x64 LTS

This issue is verified fixed on latest Nightly 53.0a1 (2016-11-22).