Introduce rendering menu items using function provided by menu context. Thread actions menu should now have correct position.
As we need to set correct position of the content in portal, I used getBoundingClientRect react function, which calculates position of rendered element.
The position needs to be updated window resizes.
Details
Details
In web app, click on thread actions menu. It should appear in correct place and in browser's inspector it will be in the portal.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Only had time for a short skim. Have one question. Looks good!
| web/components/menu.react.js | ||
|---|---|---|
| 32 ↗ | (On Diff #10377) | question: Won't this function ever only be called once? Since updatePosition's deps are []. Why not just make this dep list empty too? |
Comment Actions
Only had time for a short skim.
@benschac In that case you should make someone a blocking reviewer, so that this diff appears in someone's queue for a deeper review
| web/components/menu.react.js | ||
|---|---|---|
| 32 ↗ | (On Diff #10377) | Making so would require silencing eslint error. It's more correct to have this dependency. |
| 42–44 ↗ | (On Diff #10377) | You can use a shorthand |
| 47–54 ↗ | (On Diff #10377) | Is it intentional that this effect is called on every render? |
| 79–84 ↗ | (On Diff #10377) | We should memoize this to avoid style to change on every render |
| 85 ↗ | (On Diff #10377) | Is it correct to use this function in render? Does it have any side effects? |
Comment Actions
Rebase & use introduced function setMenuPosition from menu context to make window resizing work correctly.
Comment Actions
We're no longer using portals so please update the title and the summary.
It looks like the logic can be simplified significantly. Currently, we keep the state in two places: in the state isOpen - if the menu is opened, and in context useRenderMenu which menu is the last one. The need for the state arose from the fact that when closing the menu we need to check if we're not closing a menu rendered by some other component. But this can be avoided by checking by exposing a new method from context that takes a node and closes the menu if it is equal to the current menu.
So to simplify the state, we need to have closeMenu function in the context that takes a node and sets the state to null if the node is equal to the current menu. This change will allow us to have all the state in a single place and to reduce the duplication of logic in renderMenu calls.
| web/components/menu.react.js | ||
|---|---|---|
| 106 ↗ | (On Diff #10521) | We don't need to use fragment when there's only one component rendered |
Comment Actions
I'm not sure if I understand you correctly, but do you suggest removing isOpen completely from the state of the component? The component receives currently onChange callback which is executed with new state as a parameter, so removing isOpen from component state is impossible, isn't it?
I agree with the idea of introducing closeMenu provided by context - it will simplify the logic in the component here, and make the code more readable.
Comment Actions
Yes, the idea was to remove isOpen. Good point with onChange. I think that still it might be possible to completely remove isOpen: we can create a function e.g. openMenu that calls onChange(true) and renderMenu and a function closeMenu that calls onChange(false) and closeMenu` from context - it's not as clear as effect, but has some other advantages. But we can come back to it later and start with introducing closeMenu. It is probably a good idea to create a new diff that adds this function.
Comment Actions
Looks ok, but I have some questions inline
| web/components/menu.react.js | ||
|---|---|---|
| 48–50 ↗ | (On Diff #10593) | This might be a good idea to add a comment why it needs to be a layout effect |
| 67–69 ↗ | (On Diff #10593) | Could you explain what is the intention here? It looks like we have this effect so that the menu is closed when this component is unmounted. It is ok to have it, but it has one important issue: closeMenuCallback depends on menuActionList, so when menuActionList changes, the callback also changes and we close it. Is it intentional? It might make sense, but also an alternative, where after menuActionList changes we replace old menu with a new one also has some advantages. Curious about the intentions here. |
| web/components/menu.react.js | ||
|---|---|---|
| 67–69 ↗ | (On Diff #10593) | The aim was to remove listener and execute closeMenu when component is unmounted - e.g. when modal disappears. I didn't notice, that after menuActionList changes, the menu would disappear. Right now, it never happens, as menu action list is always the same, but how could this behavior be avoided? |
Comment Actions
This diff looks correct, but is complicated so it might be a good idea for @ashoat to take a look
| web/components/menu.react.js | ||
|---|---|---|
| 37–39 ↗ | (On Diff #10678) | You can remove the { return } part and just use () => <div /> syntax, I think |
| 43 ↗ | (On Diff #10678) | Is it possible for the result of buttonRef.current.getBoundingClientRect() to change, other than when the user resizes the screen? |
| 59–66 ↗ | (On Diff #10678) | Can we merge this useEffect into the next one? Then possibly we can get rid of currentlyRenderedMenu |
| 60–65 ↗ | (On Diff #10678) | We can avoid running the cleanup function if we don't need to, and simplify the logic a bit by early-exiting |
| 70–73 ↗ | (On Diff #10678) | I'm confused about this condition. Is the intention here "when menuActionList changes, re-render it"? If so I don't think currentlyRenderedMenu is necessary |
| 76–83 ↗ | (On Diff #10678) | Can you move this up to below where updatePosition is defined? I initially wrote a comment about updating on window resize when I first read updatePosition – I later deleted it after reading this. By localizing code we can make it more readable |
| web/components/menu.react.js | ||
|---|---|---|
| 43 ↗ | (On Diff #10678) | getBoundingClientRect() value can change if menu position changes for some reason - like e.g. some styles change and the component is moved. I didn't notice such a case in our app, and the window resize operation is the only that can affect menu position. If we'd like to handle every other case when menu button is moved (although I didn't notice any such case) we can execute updatePosition every render, but this would affect performance a bit more. |
| 70–73 ↗ | (On Diff #10678) | Not exactly - the condition here is to check if the previous action list has been rendered. currentlyRenderedMenu is a value provided by Menu Context that allows to check if the component's menu is being rendered and to confirm that this component's menu is currently displayed for the user. As we don't keep state (if it's open now) in menu component, we need currentlyRenderedMenu to check if currently displayed menu items are "our" items, which we do by comparing it in line 70. If the condition here is false, it means that the currently rendered list (by the menu context) comes from some other menu component, or there is no menu rendered now, so we don't want to render it as we may override some other's menu. If it's true - it means, that current component displays actions list now, and the list has changed, so we need to update it by executing renderMenu again. |
Comment Actions
simplify syntax, add early exit and move effect below updatePosition function following Ashoat's suggestions.
| web/components/menu.react.js | ||
|---|---|---|
| 70–73 ↗ | (On Diff #10678) | This condition is doing two things and it's very confusing.
This second condition is very difficult to understand, especially when we consider the effects of closing/reopening the Menu, and potentially another component also simultaneously manipulating the Menu. I'd like us to think a bit more about how we can make this logic more readable and clear. I sent a meeting invite for tomorrow at 11:30am ET, but if it is too late no worries – we can try to follow-up over text. Curious to get @palys-swm's perspective as well. |
Comment Actions
Talked about this in my 1:1 with Tomek today. Here's a possible solution that I think would be more readable:
- Add a new property to MenuContext called currentOpenMenu: symbol, and also setCurrentOpenMenu: symbol => void
- We want to use a Symbol because it makes it easy to generate a random unique thing that is not equal to anything else. Note that Symbol('foo') !== Symbol('foo') – details here
- Make each instance of the Menu component declare const ourSymbol = React.useRef(Symbol()), so each Menu component has its own symbol
- Make it so onClickMenuCallback just calls setCurrentOpenMenu(ourSymbol.current) and does nothing else
- Adjust the React.useEffect that calls renderMenu so that it calls renderMenu only if currentOpenMenu === ourSymbol.current and if prevActionListRef.current !== menuActionList (see inline)
- No longer need to access currentlyRenderedMenu from this component
- No longer need to procedurally trigger onChange – we can trigger this declaratively from a React.useEffect based on whether currentOpenMenu === ourSymbol.current
| web/components/menu.react.js | ||
|---|---|---|
| 79–85 ↗ | (On Diff #10741) | If you adjust the approach as I suggest, this would look like |
| web/components/menu.react.js | ||
|---|---|---|
| 69–72 ↗ | (On Diff #10819) | There are two reasons to put something inside a React.useMemo:
Since neither case applies here, I don't think we should use React.useMemo |
| web/components/menu.react.js | ||
|---|---|---|
| 76–87 ↗ | (On Diff #10863) | Should we use isOurMenuOpen here? |
| web/components/menu.react.js | ||
|---|---|---|
| 83 ↗ | (On Diff #10863) | menuActionList is still in this dep list, but is not actually needed. Can you please go through all of your dep lists and make sure they don't have anything they don't need to have? |