Closed Bug 1336382 Opened 3 years ago Closed 2 years ago

Implement RequestListContextMenu React component

Categories

(DevTools :: Netmonitor, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: rickychien, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

Architecture reactify step for creating top level NetworkMonitor react component.

* Implement RequestsListContextMenu component instead of requests-list-context-menu.js
* Remove requests-list-context-menu.js
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
Priority: -- → P3
QA Contact: ciprian.georgiu
Whiteboard: [netmonitor][triage] → [netmonitor-reserve]
Depends on: 1316291
Summary: Implement RequestListContextMenu component → Implement RequestListContextMenu React component
Duplicate of this bug: 1362054
Assignee: nobody → rchien
Blocks: 1404928
Status: NEW → ASSIGNED
After experiment, I've some different opinions of converting contextmenu into React component.

ContextMenu is built on menu.js
 * launchpad https://github.com/devtools-html/devtools-core/blob/master/packages/devtools-contextmenu/menu.js
 * toolbox https://searchfox.org/mozilla-central/source/devtools/client/framework/menu.js (using XUL native menupopup)
So that we can handle popup show & popup hide more precisely like nature contextmenu.

For example, when contextmenu is showing on screen, mouse click on an area where out of contextmenu itself can hide contextmenu, which behavior is able to be handled by menu.js properly. But if we want to convert exist requests-list-context-menu into react, it forces us to re-implement those native supported behaviors on our own. And also introduce extra state or Redux props for  contextmenu open.

React component ends up expecting an element return in render() function, and hookup the component in somewhere (in our case, the contextmenu component would be appended in RequestListContent.js). It's contrary to our case because in reality our popup element has already appended on document [1][2]. Therefore, it would cause an unnecessary setup such as rendering an extra empty div in RequestsListContextMenu's render function.

[1] https://searchfox.org/mozilla-central/source/devtools/client/framework/menu.js#67
[2] https://github.com/devtools-html/devtools-core/blob/master/packages/devtools-contextmenu/menu.js#L39
Close this issue due to agreement in today's netmonitor meeting due to comment 2.

I'll file a new bug for arranging left over files (xxx-context-menu.js...etc) under src folder.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Flags: qe-verify+
Resolution: --- → WONTFIX
Whiteboard: [netmonitor-reserve]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.