feat(cdk/menu): Allow setting cdkMenuTriggerFor null#25818
feat(cdk/menu): Allow setting cdkMenuTriggerFor null#25818crisbeto merged 5 commits intoangular:mainfrom
Conversation
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
365f3a9 to
4a77921
Compare
crisbeto
left a comment
There was a problem hiding this comment.
The change LGTM, but the CI is failing because:
- The commit title should start with
feat(, notfeature(. - You should run
yarn approve-api cdk/menuand commit the changed API golden file.
src/cdk/menu/menu-item.ts
Outdated
| /** 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.
- 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
|
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'