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

[FEAT] Possible to add keypress listeners? #19

Closed
walmat opened this issue Jul 16, 2020 · 5 comments
Closed

[FEAT] Possible to add keypress listeners? #19

walmat opened this issue Jul 16, 2020 · 5 comments

Comments

@walmat
Copy link

walmat commented Jul 16, 2020

Hi there! First off, thanks for the amazing hook into the MUI's dialog component! I've been using this in a lot of my UI and have zero problems with it.

One thing I ended up doing on my own was wrapping the confirmation in a keypress listener, so that it would confirm the dialog if the user presses enter (or another key of my choice). I feel like this would be a great addition to this as it improves UX a bit. If you'd like, I can submit a PR and you can look it over and tell me what you think.

Anyways, thanks again for putting this together! Have a great day 😄

@jonatanklosko
Copy link
Owner

Hey! Glad you find it useful, a PR is sure welcome =) One option would be adding onKeyDown here and then see if the pressed key matches what we want. As for the API perhaps a confirmationKey with one of the allowed key values?

@josiahayres
Copy link

An even easier solution would be to add the autoFocus prop onto the submit button, like how the team have done with the demo at: https://material-ui.com/components/dialogs/#alerts

@jonatanklosko
Copy link
Owner

jonatanklosko commented Sep 2, 2020

@josiahayres that's a good point! Using this approach it's actually possible to configure the behavior globally (demo). @walmat does it remove the need for your wrapper?

@walmat
Copy link
Author

walmat commented Sep 2, 2020

@josiahayres Brilliant. I'm not sure why I didn't think about that!

@jonatanklosko Yep! I think that should be mentioned (if not already) in the readme as to me it seems a pretty commonly desired functionality. Let me know if you want me to open a PR with the updated readme, I'm more than willing. 😄

@jonatanklosko
Copy link
Owner

@walmat I've just added a new section in readme, hopefully it does the job =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants