Skip to content
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

Fix(md-menu) : Added prevent close( when user clicks inside menu panel) feature. #3897

Closed
wants to merge 6 commits into from

Conversation

ravinderpayal
Copy link

@ravinderpayal ravinderpayal commented Apr 4, 2017

menu-caveate
As It's a feature the core team decided to add, and I was in need of this feature for functionality like drop-down menu having some setting controls where user may want to change multiple settings in one go. As currently after every click inside or outside of drop-down menu, it disappears. But adding some additional check based on the value of preventClose @input() supplied parent node can add this desired functionality. Actually I am creating a emoji picker, and have multiple categories of emoji, so user is required to change the category by clicking on tabs(Yeah I am using MD-tabs), menu disappears so this feature will help in accomplishing my specific requirement, and other people's similar requirements.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label Apr 4, 2017
@ravinderpayal
Copy link
Author

ravinderpayal commented Apr 4, 2017 via email

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Apr 4, 2017
@willshowell
Copy link
Contributor

Hey @ravinderpayal, as a heads up, the team will certainly ask for tests to verify the behavior of the api change.

You should also try to get your linting issues fixed before review. It looks like a couple of the comments are unnecessary / not formatted correctly.

Additionally, consider using disableClose to match the api for MdDialogConfig (not necessary, just a thought).

Finally, it would be a good idea to explain your motivation behind the change in your original comment!

@ravinderpayal
Copy link
Author

ravinderpayal commented Apr 5, 2017

@willshowell Thanks Sir, I will consider your thoughts, and thanks one more time for clearing my doubts about failure of travis build.

Added more brevity in comments, and aligned them according to the CODE STANDARDS.
satisfying travis-ci
satisfying travis
satisfying travis ci, and finally it will be satisfied
@ravinderpayal
Copy link
Author

Travis Build Passed, and @wilshowell I have kept the preventClose as it is, because disableClose mean something like completely disabling the close, while we are preventing close only when user click inside the panel.

@ravinderpayal ravinderpayal changed the title Update menu-trigger.ts Fix(md-menu) : Added prevent close( when user clicks inside menu panel) feature. Apr 5, 2017
@willshowell
Copy link
Contributor

As a few more notes, JSDoc comments (/** */) should only be used for api purposes (like preventClose and toggleMenu) while you should use standard line comments (//) elsewhere.

Again, tests will be required by the team, so you should add those too.

Good luck with your PR!

@ravinderpayal
Copy link
Author

ravinderpayal commented Apr 5, 2017 via email

@agostinalucia
Copy link

👏

@zmanring-zz
Copy link

I need this! :D Thank you @ravinderpayal

@ravinderpayal
Copy link
Author

While working on some other project I found a better option, let me patch mine code.

@ravinderpayal
Copy link
Author

@zmanring @agostinalucia @mprobst @willshowell I have created a fresh pull request after testing working of code. This current commits are not required. I have added preventClose input/attribute on <md-menu> component itself, but before it's was required to add preventClose on trigger component/element.

ezgif com-video-to-gif

#4354

@jelbourn
Copy link
Member

Closing since this is now quite stale. I think this feature has come up a few times in issue discussion and we've decided that we wouldn't want to add this option to mat-menu since it would go against W3C a11y recommendations.

@jelbourn jelbourn closed this Jan 24, 2018
@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 Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants