feat(compass-components): add context menu COMPASS-9386 #6956
Conversation
…p menu state consistent The refactor is meant to make the Leafygreen integration more straightforward: 1. We stick to item groups and instead have a single wrapper to handle any rendering differences between groups. This allows the wrapper to always have context of all items when rendering which is useful when inserting menu seperators in Leafygreen. Also encourages consistent UI (while allowing per-case customization if needed at wrapper-level). We could introduce itemWrappers instead of itemGroups but having one wrapper handling all seems cleaner to me. 2. More of the responsibility is moved to a parent wrapper component that will house the context menu. This allows us to standardize the right click menu and make better use of Leafygreen's menu component including its click handling (which has been removed from the context menu library). 3. Menu state (i.e. position) is now preserved even closed; this is useful for leafygreen's menu to animate in the same position instead of losing the position all together.
…ompass into gagik/context-menu-compass-ui
gagik
changed the title
…p menu state consistent The refactor is meant to make the Leafygreen integration more straightforward: 1. We stick to item groups and instead have a single wrapper to handle any rendering differences between groups. This allows the wrapper to always have context of all items when rendering which is useful when inserting menu seperators in Leafygreen. Also encourages consistent UI (while allowing per-case customization if needed at wrapper-level). We could introduce itemWrappers instead of itemGroups but having one wrapper handling all seems cleaner to me. 2. More of the responsibility is moved to a parent wrapper component that will house the context menu. This allows us to standardize the right click menu and make better use of Leafygreen's menu component including its click handling (which has been removed from the context menu library). 3. Menu state (i.e. position) is now preserved even closed; this is useful for leafygreen's menu to animate in the same position instead of losing the position all together.
gagik
force-pushed
the
gagik/headless-context-menu
branch
from
82a10c5 to
ca1fb86
Compare
…ompass into gagik/context-menu-compass-ui
| children: React.ReactNode; | ||
| }) { | ||
| return ( | ||
| <ContextMenuProviderBase wrapper={ContextMenu}> |
There was a problem hiding this comment.
maybe there's a better way to structure this, including the wrapper passing in general
gagik
force-pushed
the
gagik/headless-context-menu
branch
from
ca1fb86 to
ee658e1
Compare
…ompass into gagik/context-menu-compass-ui
| </CompassConnections> | ||
| </ConnectFnProvider> | ||
| </ConnectionStorageProvider> | ||
| <ContextMenuProvider> |
There was a problem hiding this comment.
Actually (and sorry for not thinking about this right away), I think we can put it into CompassComponentsProvider in compass-components package, that's probably a good place to make sure we have it everywhere consistently, including here
| export function useContextMenuItems( | ||
| items: ContextMenuItem[] | ||
| ): React.RefCallback<HTMLElement> { | ||
| const contextMenu = useContextMenu(); |
There was a problem hiding this comment.
there could be reasons for us to memoize these items? maybe we should support that at this point
There was a problem hiding this comment.
alternatively we leave that to outside of this hook
There was a problem hiding this comment.
If we don't memoize these items it looks like it will recreate all of the listeners and the ref function every time it renders. I'm not sure how new functions passed to ref perform work or cause other items to render, but it sounds like its something that would be nice to avoid by memoizing.
There was a problem hiding this comment.
good point. made it memoizable by default
| @@ -58,7 +58,10 @@ describe('ContentWithFallback', function () { | |||
| { container } | |||
| ); | |||
|
|
|||
| expect(container).to.be.empty; | |||
| expect(container.children.length).to.equal(1); | |||
There was a problem hiding this comment.
one might want to keep this empty... the reason the context menu always exists even when closed is for the sake of animations
| if (parentContext) { | ||
| throw new Error( | ||
| 'Duplicated ContextMenuProvider found. Please remove the nested provider.' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Wouldn't it make sense to move this up? (to the useContext call above) Also - this seems like an issue that we'll easily find and fix and not easily regress. What's your thoughts around limiting the use of this provider to not be nested? I mean, I cannot personally cannot think of a use for nesting them, but then again - I don't know if we need to actively prevent it.
There was a problem hiding this comment.
I was doing that to avoid breaking the rule of hooks but yeah if we're throwing it doesn't matter anyhow.
Essentially if we end up with a nested provider we'll end up with these weird double right click menus. So it does need some extra handling to support nesting (i.e. re-use parentContext and avoid registering more listeners) but we don't want to encourage this so I'd rather throw. Fine with either though.
There was a problem hiding this comment.
I thought so: Throwing is not considered a conditional branch / violation of the rules of hooks, because the throw aborts the render.
I'm not feeling strongly about the prevention of nested context providers 👍
There was a problem hiding this comment.
Sounds good, the throwing ended up useful to me when dealing with weirdness of AG Grid's isolated context (I'd get quick feedback that now I am re-nesting the provider) so I'll keep that way but simplify the throwing like you suggest
…ompass into gagik/context-menu-compass-ui
gagik
force-pushed
the
gagik/context-menu-compass-ui
branch
from
30b97a5 to
6b6e398
Compare
gagik
force-pushed
the
gagik/context-menu-compass-ui
branch
from
7e90b5f to
4dafa7b
Compare
| // by testing-library-compass already sets up the context menu provider which is not | ||
| // useful for our tests. | ||
| // eslint-disable-next-line @typescript-eslint/no-restricted-imports | ||
| export { render } from '@testing-library/react'; |
There was a problem hiding this comment.
This is related to the fact that render would otherwise wrap the context menu provider which in any case would lead to weirdness for isolated tests. I could remove the parent provider from the test but then our testing is coupled with compass-components, which is not something we want.
Since this is just related to testing, I'm going to merge or go ahead with this but as this might be a more controversial choice, if there's a follow-up needed to address this in a different way, go for it.
@kraenhansen
There was a problem hiding this comment.
Please let's not do that, it's really hard to maintain consistency for this tooling across monorepo and we moved the harness to one place for a reason (this also follows testing library recommendations for how to manage those in complex projects). We expose testingLibrary from testing harness package already for some of those rare cases where using custom render is not desireable, why is it not enough to use it here?
There was a problem hiding this comment.
thanks! I agree, did not realize there was testingLibrary thank you, that'd definitely work; did not want to ping people after hours and wanted to just have a minimal change bandaid to get all the other right click PRs in a cleaner state before going OOO.
This reverts commit 4dafa7b and uses testing-library instead.
Ties #6937's context menu with leafygreen components and exposes that.