feat(compass-context-menu): add a headless context menu package COMPASS-9386 #6937
Conversation
| export function useContextMenu({ | ||
| Menu, | ||
| }: { | ||
| Menu: React.ComponentType<{ |
There was a problem hiding this comment.
I guess we could keep it headless by letting it accept a component?
Open to a cleaner solution
gagik
force-pushed
the
gagik/headless-context-menu
branch
from
c824499 to
647306f
Compare
| (Content, index) => <Content key={`menu-content-${index}`} /> | ||
| ), | ||
| position: { | ||
| // TODO: Fix handling offset while scrolling |
There was a problem hiding this comment.
Not too sure what this was referring to, just came from the prototype
There was a problem hiding this comment.
I think it's a reminder to check how this functions if the menu is displayed inside a view which is scrolled 🤔
There was a problem hiding this comment.
When right click menu is active, should we just disable scrolling? This seems to be how context menus work in browser
There was a problem hiding this comment.
I'm tempted to agree and it seems right to lean into the pattern of disabling scrolling (although I'm surprised that this is the established pattern).
This is however an explicit non-goal of the scope 🤔 We could amend, if you see an easy way to do it .. I can't remember why I added it - likely just to keep things simple.
gagik
force-pushed
the
gagik/headless-context-menu
branch
2 times, most recently
from
31f6adf to
43023f4
Compare
gagik
force-pushed
the
gagik/headless-context-menu
branch
from
82a10c5 to
ca1fb86
Compare
…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
ca1fb86 to
ee658e1
Compare
| ); | ||
| } | ||
| if (trigger) { | ||
| trigger.addEventListener('contextmenu', listener); |
There was a problem hiding this comment.
Do we need to clean this up at all? Or is that handled whenever the dom element unmounts?
There was a problem hiding this comment.
This is adapted from @kraenhansen prototype code so he may also have more context.
I suppose it wouldn't be problematic without the cleanup but I would stick to principle of always manually handling the cleanup of a listener after adding it
| wrapper, | ||
| }: { | ||
| children: React.ReactNode; | ||
| wrapper: React.ComponentType<{ |
There was a problem hiding this comment.
I saw you mentioned this in the follow up pr: https://github.com/mongodb-js/compass/pull/6956/files#r2104832123
wrapper here could be a bit ambiguous, although we're only really consuming this in compass-components in one place, maybe we rename it to something a bit more descriptive like menuWrapper?
(Not a blocker, also feel free to make any changes we might want in that follow up)
…ompass into gagik/headless-context-menu
This acts as the headless basis for #6956 right click component
TODO: