Skip to content

fix(cdk/menu): update docs to reflect current implementation and add correct role for triggers #24884

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 12, 2022

Conversation

mmalerba
Copy link
Contributor

@mmalerba mmalerba commented May 5, 2022

No description provided.

@mmalerba mmalerba added the target: rc This PR is targeted for the next release-candidate label May 5, 2022
@mmalerba mmalerba added this to the 14.0.0 milestone May 5, 2022
@mmalerba mmalerba requested a review from jelbourn May 6, 2022 16:10
jelbourn
jelbourn previously approved these changes May 6, 2022
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, mostly minor grammar nits

jelbourn
jelbourn previously approved these changes May 9, 2022
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

One thing that occurred to me for a potential follow-up PR is documenting the CSS classes applied by the cdk menu directives

@mmalerba
Copy link
Contributor Author

I added a section about CSS classes to this PR, but its pretty boring... all of the classes but one were just static always-applied classes. I guess there's a few options for what we could do with the CSS classes:

  1. Leave it how it is, pretty much just static classes, e.g. cdk-menu, cdk-menu-trigger, etc
  2. Add more dynamic classes, e.g. cdk-menu-opened, cdk-menu-item-selected
  3. Just remove all the classes and leave it up to users to add them if they want them (all of the static ones could just be accomplished with an attribute selector anyways, e.g. [cdkMenu])

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repeating here from our conversation earlier for posterity:
I think the CSS classes are useful for both inheriting the directives and for when we can do directives on host elements

@mmalerba mmalerba added the action: merge The PR is ready for merge by the caretaker label May 12, 2022
@mmalerba mmalerba merged commit 53d7fb3 into angular:main May 12, 2022
mmalerba added a commit that referenced this pull request May 12, 2022
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker target: rc This PR is targeted for the next release-candidate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants