[web] Fix issue when opening menu without closing the previous
ClosedPublic
Actions

Authored by • jacek on May 13 2022, 4:32 AM.

Details

Reviewers

Commits

Summary

Fix problem described here:
https://linear.app/comm/issue/ENG-1125/opening-new-menu-without-closing-the-previous-one-doesnt-work
The source of the problem was closeMenu function provided by menu context, that cleared menu symbol just after it was updated by other menu component.
The diff introduces the following changes in the API:

closeMenu parameter is now menu symbol instead of node - it makes API more consistent, as setCurrentOpenMenu already operated with symbol.
closeMenu before clearing the menu, now checks if if was called from component that is active at the moment and clear menu only in that case.
to simplify managing state, menu node and symbol are merged into single state with one object inside. The main reason was to avoid executing setMenu inside callback passed to setCurrentMenu. In result, separate setMenuSumbol and setMenuNode functions were introduced, to avoid API changes.

Test Plan

Open members modal and open two menus in a row without closing first one. The issue should no longer exists.

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

• jacek requested review of this revision.May 13 2022, 4:36 AM

Adding @ashoat because the idea about using symbol came from him

Requesting changes for an answer to the inline question.

closeMenu parameter is now menu symbol instead of node - it makes API more consistent, as setCurrentOpenMenu already operated with symbol.

This was my suggestion on D3375, so I'm all for it :)

web/menu-provider.react.js
54 ↗	(On Diff #12649)	It's strange to me that we would change the `symbol` but keep the `node` the same. Should this be replaced with the suggestion?

This revision now requires changes to proceed.May 16 2022, 12:45 PM

web/menu-provider.react.js
54 ↗	(On Diff #12649)	Agree with it. Every symbol change is followed by replacing menu node. The current solution worked well because the node was replaced by executing `renderMenu` in menu component right after the symbol had changed. There is however no reason why we should render an old menu node for this short period of time, so I think the suggested solution is better.