-
Notifications
You must be signed in to change notification settings - Fork 247
chore(context-menu): use anchor ref and compact menu #7073
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the context menu to use an external anchor element for positioning, applies temporary compact styling, and updates related tests to reflect the new data-testid attributes.
- Switches from nesting the menu under its trigger to an absolutely positioned anchor ref
- Applies compact menu and item CSS overrides via Emotion
- Updates tests to query
context-menu-containerinstead of the oldcontext-menuid, and adjusts ContentWithFallback tests accordingly
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
context-menu.tsx |
Introduce anchorRef, remove useEffect, add compact CSS classes |
context-menu.spec.tsx |
Change selector to [data-testid="context-menu-container"] |
content-with-fallback.spec.tsx |
Update test description/assertion to context-menu-container and add an unused import |
Comments suppressed due to low confidence (1)
packages/compass-components/src/components/context-menu.tsx:74
- The new
menuStylesanditemStylescompact overrides aren’t currently asserted in any tests. Consider adding a test to verify that these classes are applied correctly to the menu and items.
className={menuStyles}
| if (anchorRef.current) { | ||
| anchorRef.current.style.left = `${position.x}px`; | ||
| anchorRef.current.style.top = `${position.y}px`; | ||
| } |
Copilot
AI
Jul 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating anchorRef.current.style directly on every render can lead to unnecessary DOM writes. Consider wrapping these updates in a useLayoutEffect that only runs when position changes.
| if (anchorRef.current) { | |
| anchorRef.current.style.left = `${position.x}px`; | |
| anchorRef.current.style.top = `${position.y}px`; | |
| } | |
| React.useLayoutEffect(() => { | |
| if (anchorRef.current) { | |
| anchorRef.current.style.left = `${position.x}px`; | |
| anchorRef.current.style.top = `${position.y}px`; | |
| } | |
| }, [position]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, we have to do this outside of a useEffect or useLayoutEffect for it to work.
packages/compass-components/src/components/content-with-fallback.spec.tsx
Outdated
Show resolved
Hide resolved
66beb5b to
2f57140
Compare
| menu.close(); | ||
| } | ||
| }, [menu.isOpen]); | ||
| // TODO: Remove when https://jira.mongodb.org/browse/LG-5342 is resolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to address this when that ticket is resolved, we can create a ticket marked as Blocked and link the tickets as depends upon. Then when it's resolved that ticket will get set to open and we get reminded of it.
Fine as is, I figured I'd mention in case you haven't done that flow before.
Description
Merging this PR will:
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes