feat(cdk-experimental/menu): add context menu trigger directive #20144
Conversation
googlebot
added
the
cla: yes
|
This pr implements the basic context menu logic. A follow on PR will implement keyboard triggering |
| expect(getContextMenu()).not.toBeDefined(); | ||
| }); | ||
|
|
||
| it('should re-open the menu when right clicking twice in the context', () => { |
There was a problem hiding this comment.
This test description leaves it ambiguous whether one or two menus are expected. The context string below does make it clearer, but maybe say 're-open the same menu' for extra clarity.
| */ | ||
| function isWithinMenuElement(target: Element | null) { | ||
| while (target instanceof Element) { | ||
| if (target.className.indexOf('cdk-menu') !== -1) { |
There was a problem hiding this comment.
This can break on something like class="cdk-menu-close". You should use target.classList.contains('cdk-menu') instead.
There was a problem hiding this comment.
All cdk menu classes are in the form of cdk-menu-* so we want to check if we've hit any directive not just the cdk-menu directive. However I could see users placing cdk-menu-foo on their own elements but not sure how likely that is. @jelbourn thoughts?
There was a problem hiding this comment.
My stance is that we own the cdk- prefix and users shouldn't be making up their own cdk classes. If this breaks as a result of that, it's outside the scope of our purview.
| this.opened.next(); | ||
|
|
||
| if (this._overlayRef) { | ||
| this._overlayRef!.updatePositionStrategy(this._getOverlayPositionStrategy(coordinates)); |
There was a problem hiding this comment.
Instead of recreating the entire position strategy on each open, you should be able to only update the coordinates through FlexibleConnectedPositionStrategy.setOrigin.
| /** Configuration options passed to the context menu. */ | ||
| export type ContextMenuOptions = { | ||
| /** The opened menus X coordinate offset from the triggering position. */ | ||
| menuOffsetX: number; |
There was a problem hiding this comment.
I think just offsetX and offsetY would be good since "menu" is implied
| }; | ||
|
|
||
| /** Injection token for the ContextMenu options object. */ | ||
| export const CDK_CONTEXT_MENU_OPTIONS = new InjectionToken('cdk-context-menu-options'); |
There was a problem hiding this comment.
| export const CDK_CONTEXT_MENU_OPTIONS = new InjectionToken('cdk-context-menu-options'); | |
| export const CDK_CONTEXT_MENU_OPTIONS = | |
| new InjectionToken<ContextMenuOptions >('cdk-context-menu-options'); |
| }; | ||
|
|
||
| /** Injection token for the ContextMenu options object. */ | ||
| export const CDK_CONTEXT_MENU_OPTIONS = new InjectionToken('cdk-context-menu-options'); |
There was a problem hiding this comment.
I think we generally name providers like this like CDK_CONTENT_MENU_DEFAULT_OPTIONS
| * Open the attached menu at the specified location. | ||
| * @param coordinates where to open the context menu | ||
| */ | ||
| openMenu(coordinates: ContextMenuCoordinates) { |
There was a problem hiding this comment.
| openMenu(coordinates: ContextMenuCoordinates) { | |
| open(coordinates: ContextMenuCoordinates) { |
| } | ||
|
|
||
| /** Whether the attached menu is open. */ | ||
| isMenuOpen() { |
There was a problem hiding this comment.
| isMenuOpen() { | |
| isOpen() { |
| * Get the portal to be attached to the overlay which contains the menu. Allows for the menu | ||
| * content to change dynamically and be reflected in the application. | ||
| */ | ||
| private _getPortal() { |
There was a problem hiding this comment.
| private _getPortal() { | |
| private _getMenuContent(): Portal<unknown> { |
| // stop the default context menu from appearing if user right-clicked somewhere outside of | ||
| // any context menu directive or if a user right-clicked inside of the opened menu and just | ||
| // close it. | ||
| if (event.type === 'contextmenu') { | ||
| event.preventDefault(); | ||
| this.closeMenu(); |
There was a problem hiding this comment.
I'm not sure this bit is necessary- I tested a few other Google apps (gmail, docs, calendar) and they don't seem to prevent native context menus from opening when you click somewhere without an open one (while you have an open custom menu)
There was a problem hiding this comment.
Do we want to prevent the default context menu from opening when right clicked inside a custom context menu?
There was a problem hiding this comment.
Hmm, interesting- gmail completely ignores right-clicks inside of a custom context menu while Google Docs and Calendar both just treat them as normal clicks. I would probably do what gmail does here since it seems the most predictable.
There was a problem hiding this comment.
Agreed.
So I've implemented it as:
- if regular click occurs outside of the open context menu, close the open menu
- if right click occurs inside the context menu, do nothing
- if right click occurs outside of the context menu (and outside the context) close the menu and either open the new context menu or the regular context menu
andy9775
force-pushed
the
cdk-menu-context-menu
branch
from
41cf985
to
8f41518
Compare
|
@teflonwaffles @crisbeto @jelbourn feedback should be addressed |
andy9775
force-pushed
the
cdk-menu-context-menu
branch
from
8f41518
to
d5520e5
Compare
Add a directive which opens an attached context menu when a user right clicks within the triggering element. The context menu trigger also considers nested context menu triggers within an element opting to open the lowest level non-disabled context menu.
andy9775
force-pushed
the
cdk-menu-context-menu
branch
from
d5520e5
to
dee5a8b
Compare
jelbourn
added
action: merge
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Add a directive which opens an attached context menu when a user right clicks within the
triggering element. The context menu trigger also considers nested context menu triggers within
an element opting to open the lowest level non-disabled context menu.