feat(cdk-experimental/menu): add ability to open menus from a standalone trigger #20363

andy9775 · 2020-08-19T15:11:51Z

Adds the ability to open a menu from a menu trigger placed outside of a menu or menubar.

teflonwaffles · 2020-08-19T17:15:48Z

src/cdk-experimental/menu/background-click-service.ts

+/** Injection token for CloseDecider function. */
+export const CLOSE_DECIDER = new InjectionToken<CloseDecider>('cdk-menu-close-decider');
+
+@Injectable({providedIn: 'root'})


This should have some documentation, especially since I'm a little unclear whether the client developer will call this directly, or whether it will only be internally invoked.

@teflonwaffles done

jelbourn · 2020-08-19T21:58:36Z

src/cdk-experimental/menu/background-click-service.ts

+ * out. It is provided alongside the MenuStack to the BackgroundClickService and is called on each
+ * background click event.
+ */
+export type CloseDecider = (target: Element | null) => boolean;


Suggested change

export type CloseDecider = (target: Element | null) => boolean;

export type ClosePredicate = (target: Element | null) => boolean;

https://en.wikipedia.org/wiki/Predicate_(mathematical_logic)

jelbourn · 2020-08-19T22:35:04Z

src/cdk-experimental/menu/background-click-service.ts

+ * set of menus is closed out resulting in only having a single set of open menus at any given time.
+ */
+@Injectable({providedIn: 'root'})
+export class BackgroundClickService implements OnDestroy {


I don't totally follow why this service is necessary. Why can't we use the outsidePointerEvents stream on OverlayRef for most of this, filtering the stream to the events that we care about for each type of overlay? I tried doing roughly what I had in mind here: jelbourn@584265c

What am I missing with this approach?

Agreed offline to update the Overlay logic to listen to contextmenu events (#20373) and to exclude buttons with a cdk-menu-trigger class in the predicate function.

andy9775 · 2020-08-21T22:38:14Z

@jelbourn @teflonwaffles feedback should be addressed however we're still blocked on the issue in #20377

jelbourn · 2020-08-25T18:23:28Z

src/cdk-experimental/menu/menu-item-trigger.ts

+    const isOpenTrigger =
+      coerceBooleanProperty(target.getAttribute('aria-expanded')) &&


Suggested change

const isOpenTrigger =

coerceBooleanProperty(target.getAttribute('aria-expanded')) &&

const isTriggerOpen = target.getAttribute('aria-expanded') === 'true';

I would use a direct comparison to 'true' here since coerceBooleanProperty will treat the empty string as true since it's intended to match the behavior of HTML boolean attributes

jelbourn · 2020-08-25T18:24:59Z

src/cdk-experimental/menu/menu-item-trigger.ts

+    const isOpenTrigger =
+      coerceBooleanProperty(target.getAttribute('aria-expanded')) &&
+      target.classList.contains('cdk-menu-trigger');
+    const isNoninlineMenu =


Suggested change

const isNoninlineMenu =

const isOverlayMenu =

jelbourn · 2020-08-25T18:29:06Z

src/cdk-experimental/menu/menu-item-trigger.ts

+        .outsidePointerEvents()
+        .pipe(takeUntil(this._stopOutsideClicksListener))
+        .subscribe(event => {
+          if (event.target instanceof Element && !isClickInsideMenuOverlay(event.target)) {


Is the instanceof check necessary because of something that might happen at run-time, or is it just to satisfy typescript that event.target is an element? If the latter, you can just do isClickInsideMenuOverlay(event.target as Element) since we know that something has to be an element to receive a click

andy9775 · 2020-08-25T19:20:10Z

@jelbourn feedback should be addressed.

…one trigger Adds the ability to open a menu from a menu trigger placed outside of a menu or menubar.

andy9775 · 2020-08-28T15:15:37Z

@jelbourn confirmed that this is now working now that #20377 has been merged in

jelbourn

LGTM

…one trigger (angular#20363) Adds the ability to open a menu from a menu trigger placed outside of a menu or menubar.

angular-automatic-lock-bot · 2020-09-28T00:10:49Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

andy9775 requested a review from jelbourn as a code owner August 19, 2020 15:11

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 19, 2020

andy9775 requested a review from teflonwaffles August 19, 2020 15:12

teflonwaffles reviewed Aug 19, 2020

View reviewed changes

teflonwaffles approved these changes Aug 19, 2020

View reviewed changes

andy9775 force-pushed the cdk-menu-standalone-trigger branch from 19b3e57 to ef73bff Compare August 19, 2020 19:36

jelbourn reviewed Aug 19, 2020

View reviewed changes

andy9775 force-pushed the cdk-menu-standalone-trigger branch from ef73bff to f55bcec Compare August 21, 2020 22:37

andy9775 added the blocked This issue is blocked by some external factor, such as a prerequisite PR label Aug 22, 2020

jelbourn reviewed Aug 25, 2020

View reviewed changes

andy9775 force-pushed the cdk-menu-standalone-trigger branch from f55bcec to 880309d Compare August 25, 2020 19:20

andy9775 force-pushed the cdk-menu-standalone-trigger branch from 880309d to 82bc091 Compare August 25, 2020 19:21

feat(cdk-experimental/menu): add ability to open menus from a standal…

fca424b

…one trigger Adds the ability to open a menu from a menu trigger placed outside of a menu or menubar.

andy9775 removed the blocked This issue is blocked by some external factor, such as a prerequisite PR label Aug 28, 2020

andy9775 force-pushed the cdk-menu-standalone-trigger branch from 82bc091 to fca424b Compare August 28, 2020 15:14

jelbourn approved these changes Aug 28, 2020

View reviewed changes

jelbourn added action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release merge safe labels Aug 28, 2020

jelbourn merged commit de98466 into angular:master Aug 28, 2020

annieyw pushed a commit to annieyw/components that referenced this pull request Aug 31, 2020

feat(cdk-experimental/menu): add ability to open menus from a standal…

cb99eb9

…one trigger (angular#20363) Adds the ability to open a menu from a menu trigger placed outside of a menu or menubar.

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cdk-experimental/menu): add ability to open menus from a standalone trigger #20363

feat(cdk-experimental/menu): add ability to open menus from a standalone trigger #20363

andy9775 commented Aug 19, 2020

teflonwaffles Aug 19, 2020

andy9775 Aug 19, 2020

jelbourn Aug 19, 2020

jelbourn Aug 19, 2020

andy9775 Aug 20, 2020 •

edited

Loading

andy9775 commented Aug 21, 2020

jelbourn Aug 25, 2020

jelbourn Aug 25, 2020

jelbourn Aug 25, 2020

andy9775 commented Aug 25, 2020

andy9775 commented Aug 28, 2020

jelbourn left a comment

angular-automatic-lock-bot bot commented Sep 28, 2020

	export type CloseDecider = (target: Element \| null) => boolean;
	export type ClosePredicate = (target: Element \| null) => boolean;

		const isOpenTrigger =
		coerceBooleanProperty(target.getAttribute('aria-expanded')) &&

	const isOpenTrigger =
	coerceBooleanProperty(target.getAttribute('aria-expanded')) &&
	const isTriggerOpen = target.getAttribute('aria-expanded') === 'true';

feat(cdk-experimental/menu): add ability to open menus from a standalone trigger #20363

feat(cdk-experimental/menu): add ability to open menus from a standalone trigger #20363

Conversation

andy9775 commented Aug 19, 2020

teflonwaffles Aug 19, 2020

Choose a reason for hiding this comment

andy9775 Aug 19, 2020

Choose a reason for hiding this comment

jelbourn Aug 19, 2020

Choose a reason for hiding this comment

jelbourn Aug 19, 2020

Choose a reason for hiding this comment

andy9775 Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

andy9775 commented Aug 21, 2020

jelbourn Aug 25, 2020

Choose a reason for hiding this comment

jelbourn Aug 25, 2020

Choose a reason for hiding this comment

jelbourn Aug 25, 2020

Choose a reason for hiding this comment

andy9775 commented Aug 25, 2020

andy9775 commented Aug 28, 2020

jelbourn left a comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Sep 28, 2020

andy9775 Aug 20, 2020 •

edited

Loading