Page MenuHomePhabricator

[web] Introduce menu context for displaying actions menu
ClosedPublic

Authored by jacek on Mar 9 2022, 3:45 AM.
Tags
None
Referenced Files
F3486391: D3375.diff
Wed, Dec 18, 4:46 AM
Unknown Object (File)
Sun, Dec 1, 10:46 PM
Unknown Object (File)
Mon, Nov 25, 11:46 AM
Unknown Object (File)
Wed, Nov 20, 4:30 PM
Unknown Object (File)
Tue, Nov 19, 2:58 AM
Unknown Object (File)
Tue, Nov 19, 2:58 AM
Unknown Object (File)
Nov 17 2024, 8:24 AM
Unknown Object (File)
Nov 5 2024, 9:20 PM

Details

Summary

As we need to display menu on the top of UI, I created React context that can handle rendering menu items.
The diff introduces the context that provides function allowing to render items in portal.
Rendering in the portal is not used yet. It will be in the following diffs.

Test Plan

No behavior change in web app. Empty portal div should appear in DOM as a child of react-root div. The portal can be tested after introducing rendering in one of the following diffs.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested changes to this revision.Mar 10 2022, 3:58 AM
tomek added inline comments.
web/menu-portal-provider.react.js
13 ↗(On Diff #10188)

Is it necessary to expose this ref via context? And if it's necessary, we need to have a more concrete type. (The fact that mixed works here suggests that it is not used anywhere)

34–39 ↗(On Diff #10188)

You can use a shorthand

51–56 ↗(On Diff #10188)

It might be easier to have a useRenderInMenuPortal that returns just renderInMenuPortal function

This revision now requires changes to proceed.Mar 10 2022, 3:58 AM
benschac added inline comments.
web/menu-portal-provider.react.js
53 ↗(On Diff #10188)

Can we make this more descriptive? Like: MenuPortalContext not found

web/menu-portal.css
1 ↗(On Diff #10188)

Is the portal supposed to remain off the page completely or just the top left corner of the element's closest relative parent?

web/menu-portal-provider.react.js
13–14 ↗(On Diff #10188)

I think we should avoid having any writeable properties in a context type. The risk is that a consumer of the context could directly modify some of these properties.

Agree with all of @palys-swm's comments:

  1. We should not expose a ref via context
  2. It appears the field is not used, since you typed it as mixed
  3. We should generally avoid mixed unless we explicitly want to say "anything goes" (eg. an API that takes a callback whose return value does not matter to that API)

On top of those comments, let's make sure to set every field here as $ReadOnly, including any nested fields

web/menu-portal.css
1 ↗(On Diff #10188)

It is supposed to remain in top left corner of the page (so the portal content could be styled with relative position to the .portal div using JS calculated coordinates)

Followed suggestions from review

going to resign as a reviewer since both ashoat and tomek have feedback.

ashoat requested changes to this revision.Mar 14 2022, 9:40 PM
ashoat added inline comments.
web/menu-portal-provider.react.js
28 ↗(On Diff #10355)

Do we actually need ReactDOM.createPortal here? My understanding is that it's only necessary for "brownfield" apps where the "portal" is outside of the root React component. But in our case, we have a "greenfield" app where pretty much the whole thing (and certainly <div ref={portalRef} className={css.portal}></div> below) is inside the root React component. So we can use a pseudo-portal (eg. portal design, but not an actual portal), similar to how @benschac refactored the Modal component recently. Let me know if I'm missing something here!

This revision now requires changes to proceed.Mar 14 2022, 9:40 PM
web/menu-portal-provider.react.js
28 ↗(On Diff #10355)

Good point. The initial idea was to use the context providing the ref for portal, and executing createPortal from components deep inside the root. However, after the changes, the logic for writing into the portal was moved into the context itself, which seems to have no sense.
I'll use the approach similar to the Modal instead.

removed portal and used approach from Modal

rebase & changed type

tomek added inline comments.
web/menu-provider.react.js
12–14 ↗(On Diff #10434)

We have a type that can express this

17–21 ↗(On Diff #10434)

We're adding a default value, so maybe this can be typed as React.Context<MenuContextType>?

41–43 ↗(On Diff #10434)
This revision is now accepted and ready to land.Mar 17 2022, 2:46 AM

Good catches @palys-swm, I totally missed those!

Follow review comments & introduce setMenuPosition function into context.

The reason is that previous solution required changing renedered menu object every time browser window resizes (because the position calculated in JS was kept in the menu itself).
Right now, the position updates (like on window resize) don't require changing passed menu object, which improves performance and prevents recreating all callbacks dependent on menu object (in Menu component)

This revision is now accepted and ready to land.Mar 20 2022, 9:00 PM

Added closeMenu and currentMenu into context, to simplify Menu compoenent in the following diff.

jacek retitled this revision from [web] Introduce portal for displaying actions menu to [web] Introduce menu context for displaying actions menu.Mar 22 2022, 5:16 AM
jacek edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Mar 22 2022, 5:47 AM

Include currentOpenMenu symbol in context, following the approach suggested in D3377

ashoat added inline comments.
web/menu-provider.react.js
46 ↗(On Diff #10818)

Should this take the symbol instead of menuToClose? Pro: we will be consistent with other logic, and we can protect a potential case where the React.Node is out-of-sync (the symbol for a component will always be the same across render cycles). Con: we can't pass a callback to setMenu that looks at the old value of oldMenu – we will need to have this React.useCallback take currentOpenMenu as a dependency

This revision is now accepted and ready to land.Mar 30 2022, 10:57 PM
web/menu-provider.react.js
46 ↗(On Diff #10818)

I think, the current solution is better, as we don't have any dependencies in the callback.
If the callback needed to be recomputed every time when currentOpenMenu changes, we would need to handle it in menu-react.js in the other way, as the cleaning useEffect in Menu component (when it is unmounted) and document event listener depend on this closeMenu function currently