Closed
Bug 1309183
Opened 8 years ago
Closed 8 years ago
Replace XUL Splitter by SplitBox
Categories
(DevTools :: Netmonitor, defect, P1)
DevTools
Netmonitor
Tracking
(firefox52 affected, firefox54 verified)
People
(Reporter: rickychien, Assigned: rickychien)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Whiteboard: [netmonitor])
Attachments
(1 file, 1 obsolete file)
Make sure we actually need the sidebar (Edit & Resend, WS frames in the future)
It depends on new UX spec, let’s discuss on Bug 1307667.
Updated•8 years ago
|
Whiteboard: [netmonitor]
Updated•8 years ago
|
Flags: qe-verify+
QA Contact: ciprian.georgiu
Updated•8 years ago
|
Priority: -- → P2
Comment 1•8 years ago
|
||
We should also make sure the that vertical mode is working as expected. It's currently broken and the Sidebar (details view) is taking the entire area (making the request list hidden).
Honza
Updated•8 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 2•8 years ago
|
||
browser_net_prefs-reload.js is going to disable temporarily in bug 1317645 because width and height of network-details-panel should be tackled in SplitBox.
Please remember to tackle and re-enable browser_net_prefs-reload.js along with this patch.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Comment 3•8 years ago
|
||
Updated•8 years ago
|
Iteration: --- → 54.2 - Feb 20
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•8 years ago
|
||
This patch includes:
* Create a MonitorPanel react component as alternative <vbox id="inspector-panel">...</vbox> element. The original name has renamed to inspector-panel, but feel free to provide any better naming suggestions.
* Marked bug 1336381 as duplicate.
* Moved networkDetailsWidth and networkDetailsHeight Prefs initialize/destroy and updating formDataSections from RequestList to MonitorPanel.
* Moved toolbar filters Prefs to Toolbar component.
* Removed XUL splitter workaround which was introduced in network-details-panel.js.
* Removed height: calc(100vh - 24px); which was introduced by custom request panel.
* A few fixes is supposed to be done in https://bugzilla.mozilla.org/show_bug.cgi?id=1336383#c7, but in fact I forgot to include these fixes in bug 1336383 after addressing patch conflict. So I merge them into this patch. You will see request-list-context-menu.js and request-list-tooltip.js moved to netmonitor folder.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•8 years ago
|
||
Here are several warning I saw after applied the patch, otherwise it works well
Warning: Failed prop type: Invalid prop `maxSize` of type `string` supplied to `SplitBox`, expected `number`.
in SplitBox (created by MonitorPanel)
in MonitorPanel (created by Connect(MonitorPanel))
in Connect(MonitorPanel)
in Provider react-dev.js:22807:9
Warning: Failed prop type: Invalid prop `requestFilterTypes` of type `array` supplied to `Toolbar`, expected `object`.
in Toolbar (created by Connect(Toolbar))
in Connect(Toolbar) (created by MonitorPanel)
in div (created by MonitorPanel)
in MonitorPanel (created by Connect(MonitorPanel))
in Connect(MonitorPanel)
in Provider
Warning: Failed prop type: Invalid prop `children` of type `array` supplied to `Tabbar`, expected `object`.
in Tabbar (created by TabboxPanel)
in TabboxPanel (created by Connect(TabboxPanel))
in Connect(TabboxPanel) (created by NetworkDetailsPanel)
in div (created by NetworkDetailsPanel)
in NetworkDetailsPanel (created by Connect(NetworkDetailsPanel))
in Connect(NetworkDetailsPanel) (created by MonitorPanel)
in div (created by SplitBox)
in div (created by SplitBox)
in SplitBox (created by MonitorPanel)
in div (created by MonitorPanel)
in MonitorPanel (created by Connect(MonitorPanel))
in Connect(MonitorPanel)
in Provider react-dev.js:22807:9
"Warning: Each child in an array or iterator should have a unique "key" prop. Check the render method of `TreeView`. See https://fb.me/react-warning-keys for more information.
in tr (created by TreeView)
in TreeView (created by PropertiesView)
in div (created by PropertiesView)
in div (created by PropertiesView)
in PropertiesView (created by ResponsePanel)
in div (created by ResponsePanel)
in ResponsePanel (created by TabboxPanel)
in div (created by Panel)
in Panel (created by TabboxPanel)
in div (created by Tabs)
in div (created by Tabs)
in div (created by Tabs)
in Tabs (created by Tabbar)
in div (created by Tabbar)
in Tabbar (created by TabboxPanel)
in TabboxPanel (created by Connect(TabboxPanel))
in Connect(TabboxPanel) (created by NetworkDetailsPanel)
in div (created by NetworkDetailsPanel)
in NetworkDetailsPanel (created by Connect(NetworkDetailsPanel))
in Connect(NetworkDetailsPanel) (created by MonitorPanel)
in div (created by SplitBox)
in div (created by SplitBox)
in SplitBox (created by MonitorPanel)
in div (created by MonitorPanel)
in MonitorPanel (created by Connect(MonitorPanel))
in Connect(MonitorPanel)
in Provider"
Comment hidden (mozreview-request) |
Comment 16•8 years ago
|
||
mozreview-review |
Comment on attachment 8836457 [details]
Bug 1309183 - Replace XUL Splitter by SplitBox
https://reviewboard.mozilla.org/r/111870/#review113534
::: devtools/client/netmonitor/components/monitor-panel.js:40
(Diff revision 8)
> + */
> +const MonitorPanel = createClass({
> + displayName: "MonitorPanel",
> +
> + propTypes: {
> + isEmpty: PropTypes.bool,
missing .isRequired
::: devtools/client/netmonitor/components/monitor-panel.js:109
(Diff revision 8)
> + }
> + },
> +
> + onLayoutChange() {
> + this.setState({
> + vertical: MediaQueryList.matches,
`isVertical` is more clear when we check `this.state.isVertical`to save width or height Prefs
::: devtools/client/netmonitor/components/monitor-panel.js:119
(Diff revision 8)
> + // Allow requests to settle down first.
> + setNamedTimeout("resize-events", 50, () => {
> + const waterfallHeaderEl =
> + document.querySelector("#requests-menu-waterfall-header-box");
> + if (waterfallHeaderEl) {
> + const { width } = waterfallHeaderEl.getBoundingClientRect();
this part looks like XUL, could we put them into helper or somewhere, therefore we can handle XUL without touch this component again?
::: devtools/client/netmonitor/components/request-list.js:23
(Diff revision 8)
> const { div } = DOM;
>
> /**
> * Request panel component
> */
> -const RequestList = createClass({
> +function RequestList({ isEmpty }) {
nit: { isEmpty }
::: devtools/client/netmonitor/netmonitor-view.js:60
(Diff revision 8)
> this.unsubscribeStore();
> },
>
> toggleFrontendMode: function () {
> if (gStore.getState().ui.statisticsOpen) {
> this._body.selectedPanel = document.querySelector("#react-statistics-panel-hook");
this._body.selectedPanel = this.statisticsPanel;
::: devtools/client/netmonitor/netmonitor-view.js:63
(Diff revision 8)
> toggleFrontendMode: function () {
> if (gStore.getState().ui.statisticsOpen) {
> this._body.selectedPanel = document.querySelector("#react-statistics-panel-hook");
> NetMonitorController.triggerActivity(ACTIVITY_TYPE.RELOAD.WITH_CACHE_ENABLED);
> } else {
> - this._body.selectedPanel = document.querySelector("#inspector-panel");
> + this._body.selectedPanel = document.querySelector("#react-monitor-panel-hook");
this._body.selectedPanel = this.monitorPanel;
::: devtools/client/themes/netmonitor.css
(Diff revision 8)
> /* Custom request view */
>
> .custom-request-panel {
> overflow: auto;
> - /* Full view hight - toolbar height */
> + padding: 0 4px;
> - height: calc(100vh - 24px);
The padding is changed, and scroll bar is missing without that setting
Assignee | ||
Comment 17•8 years ago
|
||
Fred, thanks for reporting warnings and warnings are addressed.
Some of these warnings didn't introduced by this patch, but you found it while reaching / triggering these UI components. It's hard to find out all warnings since it needs to navigate all UIs in runtime.
However, it would be easier for us to clean it up in one sweep (open a new issue on Github) after Netmonitor moved to GitHub and enable Flow [1] to do static analysis.
[1] https://flowtype.org/docs/react.html
Assignee | ||
Comment 18•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8836457 [details]
Bug 1309183 - Replace XUL Splitter by SplitBox
https://reviewboard.mozilla.org/r/111870/#review113534
> this part looks like XUL, could we put them into helper or somewhere, therefore we can handle XUL without touch this component again?
All of them are standard WebAPIs, it doesn't look like XUL at all. But I noticed this part doesn't belong here, I'll move them to right place. Thanks!
> nit: { isEmpty }
I'm using { isEmpty } here, what do you mean?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 21•8 years ago
|
||
mozreview-review |
Comment on attachment 8836457 [details]
Bug 1309183 - Replace XUL Splitter by SplitBox
https://reviewboard.mozilla.org/r/111870/#review113802
Excellent work here!
Honza
::: devtools/client/netmonitor/components/monitor-panel.js:30
(Diff revision 8)
> +const NetworkDetailsPanel = createFactory(require("../shared/components/network-details-panel"));
> +const RequestList = createFactory(require("./request-list"));
> +const Toolbar = createFactory(require("./toolbar"));
> +
> +const { div } = DOM;
> +const MediaQueryList = window.matchMedia("(min-width: 700px)");
Nice utilization of the matchMedia() API!
::: devtools/client/netmonitor/reducers/ui.js:11
(Diff revision 8)
>
> const I = require("devtools/client/shared/vendor/immutable");
> const {
> OPEN_NETWORK_DETAILS,
> OPEN_STATISTICS,
> + REMOVE_SELECTED_CUSTOM_REQUEST,
How come this wasn't there before?
Attachment #8836457 -
Flags: review?(odvarko) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 23•8 years ago
|
||
There is a conflict in autoland, so I'll trigger this land after m-c updated.
Updated•8 years ago
|
Attachment #8835438 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 26•8 years ago
|
||
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0df3f1f92052
Replace XUL Splitter by SplitBox r=Honza
Comment 27•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Comment 30•8 years ago
|
||
This issue is verified fixed on latest Nightly 54.0a1 (2017-03-05) under the following OSes:
- Windows 10 x64
- Mac OS X 10.11.6
- Ubuntu 16.04 x64 LTS
The SplitBox is working as expected and, also the vertical mode problem mentioned in comment 1 is fixed.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•