Closed Bug 1308426 Opened 3 years ago Closed 3 years ago

Implement toolbar for netmonitor panel

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox53 verified)

VERIFIED FIXED
Firefox 53
Iteration:
53.1 - Nov 28
Tracking Status
firefox53 --- verified

People

(Reporter: steveck, Assigned: rickychien)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [netmonitor])

Attachments

(2 files)

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
Summary: Migrate Net Panel Toolbar → Migrate Net Panel Toolbar to shared component in NetMonitor panel
Whiteboard: [devtools-html]
Depends on: 1309191
Depends on: 1309192
Depends on: 1309193
Depends on: 1309194
Whiteboard: [netmonitor]
Flags: qe-verify+
QA Contact: ciprian.georgiu
Blocks: 1309498
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Priority: -- → P1
Assignee: rchien → nobody
Status: ASSIGNED → NEW
Flags: qe-verify+
Keywords: meta
Priority: P1 → --
QA Contact: ciprian.georgiu
Summary: Migrate Net Panel Toolbar to shared component in NetMonitor panel → [meta] Migrate Net Panel Toolbar to shared component in NetMonitor panel
Whiteboard: [netmonitor]
Depends on: 1311591
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Hi Ricky, this bug was turned into a meta.
Flags: needinfo?(rchien)
Depends on: 1316484
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?
Flags: needinfo?(rchien)
(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.
Iteration: --- → 52.3 - Nov 14
Flags: qe-verify+
Keywords: meta
Priority: -- → P1
QA Contact: ciprian.georgiu
Summary: [meta] Migrate Net Panel Toolbar to shared component in NetMonitor panel → Migrate Net Panel Toolbar to shared component in NetMonitor panel
Whiteboard: [netmonitor]
Iteration: 52.3 - Nov 14 → 53.1 - Nov 28
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 :)
Summary: Migrate Net Panel Toolbar to shared component in NetMonitor panel → Implement toolbar for netmonitor panel
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)
Attachment #8811789 - Flags: review?(odvarko)
Attached image sidebar.png
One more thing, label for side-panel section-header is now aligned to the right, see the attached screenshot.

Honza
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 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
Attachment #8811789 - Flags: review?(odvarko) → review+
(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 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.
Attachment #8811789 - Flags: review+
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.
Pushed by jsnajdr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b36b0dde3499
Implement netmonitor toolbar r=Honza,jsnajdr
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b36b0dde3499
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
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).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.