feat(cdk/menu): Allow setting cdkMenuTriggerFor null #25818
Conversation
klinki
force-pushed
the
feature/port-pr-24437-to-cdk
branch
from
cb5afb4 to
365f3a9
Compare
src/cdk/menu/menu-trigger.spec.ts
Outdated
| /** run change detection and, extract and set the rendered elements */ | ||
| const detectChanges = () => { | ||
| fixture.detectChanges(); | ||
| grabElementsForTesting(); |
There was a problem hiding this comment.
I don't think that you need to call this after each change detection. The element is going to stay the same after ngAfterContentInit.
|
|
||
| @Component({ | ||
| template: ` | ||
| <button [cdkMenuTriggerFor]="null">First</button> |
There was a problem hiding this comment.
I don't think that we need an entirely separate fixture where it is set to null. Instead we should adapt an existing fixture so we can also test that the DOM is updated on dynamic changes. See how it's done for MatMenu https://github.com/angular/components/blob/main/src/material/menu/menu.spec.ts#L92.
There was a problem hiding this comment.
That's good idea, I updated tests.
Add ability to set [cdkMenuTrigger]="null" as is now possible with material menu. Fixes angular#25782
klinki
force-pushed
the
feature/port-pr-24437-to-cdk
branch
from
365f3a9 to
4a77921
Compare
src/cdk/menu/menu-item.ts
Outdated
| @@ -93,7 +93,13 @@ export class CdkMenuItem implements FocusableOption, FocusableElement, Toggler, | |||
| @Output('cdkMenuItemTriggered') readonly triggered: EventEmitter<void> = new EventEmitter(); | |||
|
|
|||
| /** Whether the menu item opens a menu. */ | |||
| readonly hasMenu = !!this._menuTrigger; | |||
| get hasMenu() { | |||
| if (this._menuTrigger?.menuTemplateRef == null) { | |||
There was a problem hiding this comment.
nit: this can be simplified to return this._menuTrigger?.menuTemplateRef != null.
klinki
changed the title
- Update hasMenu - Add yarn approve-api cdk/menu - Uncomment xdescribe and simplify it only for cdkMenuTrigger
src/cdk/menu/menu-trigger.spec.ts
Outdated
| @@ -1,3 +1,4 @@ | |||
| import {CdkContextMenuTrigger} from './context-menu-trigger'; | |||
There was a problem hiding this comment.
the linter is complaining that this import is unused
Remove unused import
crisbeto
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. |
This PR fixes issue #25782 and allows to set
[cdkMenuTriggerFor]='null'