feat(compass-components): add context menu COMPASS-9386 #6956

gagik · 2025-05-23T15:11:43Z

Ties #6937's context menu with leafygreen components and exposes that.

…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

…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 · 2025-05-23T15:27:57Z

packages/compass-components/src/components/context-menu.tsx

+  children: React.ReactNode;
+}) {
+  return (
+    <ContextMenuProviderBase wrapper={ContextMenu}>


maybe there's a better way to structure this, including the wrapper passing in general

gribnoysup · 2025-06-10T15:47:33Z

configs/testing-library-compass/src/index.tsx

-                      </CompassConnections>
-                    </ConnectFnProvider>
-                  </ConnectionStorageProvider>
+                  <ContextMenuProvider>


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

gagik · 2025-06-10T15:54:33Z

packages/compass-components/src/components/context-menu.tsx

+export function useContextMenuItems(
+  items: ContextMenuItem[]
+): React.RefCallback<HTMLElement> {
+  const contextMenu = useContextMenu();


there could be reasons for us to memoize these items? maybe we should support that at this point

alternatively we leave that to outside of this hook

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.

good point. made it memoizable by default

gagik · 2025-06-10T16:31:12Z

packages/compass-components/src/components/content-with-fallback.spec.tsx

@@ -58,7 +58,10 @@ describe('ContentWithFallback', function () {
      { container }
    );

-    expect(container).to.be.empty;
+    expect(container.children.length).to.equal(1);


one might want to keep this empty... the reason the context menu always exists even when closed is for the sake of animations

kraenhansen · 2025-06-18T08:37:32Z

packages/compass-context-menu/src/context-menu-provider.tsx

+  if (parentContext) {
+    throw new Error(
+      'Duplicated ContextMenuProvider found. Please remove the nested provider.'
+    );
+  }


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.

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.

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 👍

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

…xt-menu-compass-ui

…ess-context-menu

…ompass into gagik/context-menu-compass-ui

…xt-menu-compass-ui

gagik · 2025-06-19T15:57:44Z

packages/compass-context-menu/src/render.ts

+// 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';


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

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?

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.

kraenhansen · 2025-06-25T09:57:55Z

packages/compass-crud/src/components/document-list-view.spec.tsx

+      let component: ReactWrapper;
+      beforeEach(function () {
+        component = mount(
+          <DocumentListView
+            docs={hadronDocs}
+            isEditable={false}
+            isTimeSeries={false}
+          />,
+          { wrappingComponent: ContextMenuProvider }
+        );
+      });
+
+      afterEach(function () {
+        component?.unmount();
+      });


I wonder why this was needed and if it accidentally crept in from another PR? cc @gagik (tagging you for when you get back - it's not urgent)

kraenhansen · 2025-06-27T09:53:20Z

packages/compass-components/src/components/context-menu.spec.tsx

+import { useContextMenuItems, ContextMenu } from './context-menu';
+import type { ContextMenuItem } from '@mongodb-js/compass-context-menu';
+
+describe('useContextMenuItems', function () {


We might be missing a test to ensure the menu closes once clicked?

kraenhansen · 2025-06-27T09:58:38Z

packages/compass-components/src/components/context-menu.tsx

+      }}
+    >
+      <Menu
+        renderMode="inline"


As per the popover docs:

RenderMode.Inline and RenderMode.Portal are marked deprecated and will eventually lose support. All overlay elements should migrate to using the top layer.

I think it might make sense to just fallback on the default value for this prop ('top-layer')

Also, instead of moving the container around, it might make more sense to pass the refEl prop pointing to an element we reposition instead.

gagik added 14 commits May 22, 2025 09:38

feat(compass-context-menu): add a headless context menu package

297a943

wip

6838513

fix: add tests

8e30a83

fix: add tests and fix types

7d17984

refactor: minor stylistic changes

6004593

fix: export types and rename MenuItem

43023f4

fix: basic UI implementation

971b0fd

fix: use React.RefCallback

0ad7a63

fix: switch to item-based organization

aa9f0cf

fix: cleanup and switch to menu prop

58df56a

Merge branch 'gagik/headless-context-menu' of github.com:mongodb-js/c…

731ac13

…ompass into gagik/context-menu-compass-ui

fix: revert test environment

56d11b6

fix: justify start to make it prefer right way popups and remove comment

743f068

github-actions bot added the feat label May 23, 2025

gagik changed the title ~~feat(compass-components): add context menu~~ feat(compass-components): add context menu COMPASS-9386 May 23, 2025

gagik added the no release notes Fix or feature not for release notes label May 23, 2025

gagik added 10 commits May 23, 2025 17:19

fix: remove redundant context import

ab14b78

feat(compass-context-menu): add a headless context menu package

1d279ca

wip

1efdb2e

fix: add tests

0f5303b

fix: add tests and fix types

33b2a41

refactor: minor stylistic changes

4a2f032

fix: export types and rename MenuItem

8e1feb3

fix: use React.RefCallback

1f55000

fix: delete redundant context menu

ca1fb86

gagik force-pushed the gagik/headless-context-menu branch from 82a10c5 to ca1fb86 Compare May 23, 2025 15:20

Merge branch 'gagik/headless-context-menu' of github.com:mongodb-js/c…

a285d63

…ompass into gagik/context-menu-compass-ui

gagik commented May 23, 2025

View reviewed changes

gribnoysup reviewed Jun 10, 2025

View reviewed changes

gagik commented Jun 10, 2025

View reviewed changes

gagik added 3 commits June 10, 2025 18:04

fix: move to compass components provider

220a300

fix: remove unintended deletion

74f3d6e

fix: adjust tests

4f05381

gagik commented Jun 10, 2025

View reviewed changes

kraenhansen reviewed Jun 18, 2025

View reviewed changes

gagik added 3 commits June 18, 2025 11:15

Merge branch 'main' of github.com:mongodb-js/compass into gagik/conte…

4ad7231

…xt-menu-compass-ui

Merge branch 'main' of github.com:mongodb-js/compass into gagik/headl…

cbb0656

…ess-context-menu

Merge branch 'gagik/headless-context-menu' of github.com:mongodb-js/c…

c6d7b03

…ompass into gagik/context-menu-compass-ui

gagik requested a review from a team as a code owner June 18, 2025 09:20

fix: throw early on

6b6e398

addaleax approved these changes Jun 18, 2025

View reviewed changes

Base automatically changed from gagik/headless-context-menu to main June 19, 2025 07:52

gagik force-pushed the gagik/context-menu-compass-ui branch from 30b97a5 to 6b6e398 Compare June 19, 2025 08:06

gagik added 4 commits June 19, 2025 10:08

Merge branch 'main' of github.com:mongodb-js/compass into gagik/conte…

17c05f0

…xt-menu-compass-ui

fix: correct wrapper use

6dadf25

Merge branch 'main' of github.com:mongodb-js/compass into gagik/conte…

a62690b

…xt-menu-compass-ui

fix: use render directly for compass-context-menu

4dafa7b

gagik force-pushed the gagik/context-menu-compass-ui branch from 7e90b5f to 4dafa7b Compare June 19, 2025 15:55

gagik commented Jun 19, 2025

View reviewed changes

gagik added 4 commits June 19, 2025 18:21

fix: use testingLibrary's render

915c597

This reverts commit 4dafa7b and uses testing-library instead.

feat: memoize items for context menu

8f306e4

fix: adjust dependencies

aa52fb7

fix: support nesting

fb9c599

kraenhansen reviewed Jun 25, 2025

View reviewed changes

Merge branch 'main' into gagik/context-menu-compass-ui

b684e85

kraenhansen approved these changes Jun 25, 2025

View reviewed changes

kraenhansen reviewed Jun 27, 2025

View reviewed changes

feat(compass-components): add context menu COMPASS-9386 #6956

Are you sure you want to change the base?

feat(compass-components): add context menu COMPASS-9386 #6956

Uh oh!

Conversation

gagik commented May 23, 2025

Uh oh!

gagik May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gagik Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kraenhansen Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gagik Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribnoysup Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gagik May 23, 2025 •

edited

Loading

gagik Jun 18, 2025 •

edited

Loading

kraenhansen Jun 18, 2025 •

edited

Loading

gagik Jun 19, 2025 •

edited

Loading

gribnoysup Jun 19, 2025 •

edited

Loading