f3oall / awesome-notifications

Lightweight JavaScript Notifications Library
https://f3oall.github.io/awesome-notifications/
MIT License
286 stars 40 forks source link

Add keyboard shortcuts to popups #16

Closed andreasvirkus closed 5 years ago

andreasvirkus commented 5 years ago

Backstory: We'd love to see Enter and Escape keyboard shortcuts for the modals/popups displayed by awn. Users often expect to use Esc/Enter keys to interact with modals and atm the focus stays on the trigger element (e.g. click Run Example in this official example) and when I press Enter after triggering the popup, another popup is displayed on top (further darkening the background). This can go on forever - imagine a confused user spamming away at their Enter/Escape keys.

TODO: I wasn't sure of the correct way to call the confirm event, so please see the TODO item on line #45 and help me out a bit 🙏

Overall we really love using this library and we'd appreciate this feature immensely!

andreasvirkus commented 5 years ago

Should I access the confirm button via ${mConsts.ids.confirmOk}? For example:

const confirmBtn = event.target.closest(`${mConsts.klass.content}`).getElementById(`${mConsts.ids.confirmOk}`)
confirmBtn.click()

(searching through klass.content since a search through the whole document could be a bit long)

Edit: not sure which selector is better to find the root of the popup - mConsts.klass.content vs mConsts.klass.body?

f3oall commented 5 years ago

Hey man, thanks for you interest in this project.

Unfortunately, until I didn't add Code of conduct I won't accept any pull requests. However feature that you asked for will be implemented next week. I can share code with you after feature will be realized, so you can compare with yours.

PS. This is pretty tough task, and it requires several changes in several places of the code. Especially it becomes tough when you need to write less code. So it's kinda bad first issue. If you want to participate in this project, try to oversee the issue list. I'm sure soon there will be an issue that can be a good start for you.

PSS. I'm gonna start new project for Vue.js in few month. It will be a components library. I noticed you have Vue.js in your repo list. So any help there will be appreciated. Stay tuned.

andreasvirkus commented 5 years ago

Thanks for replying so quickly. I'll do my best to understand your reasons and look forward to seeing the final implementation.

Please let me know when the feature is released (and any updates on the components lib is also welcome) :)

f3oall commented 5 years ago

Thanks for replying so quickly. I'll do my best to understand your reasons and look forward to seeing the final implementation.

Please let me know when the feature is released (and any updates on the components lib is also welcome) :)

This is the commit. Don't forget to update to 3.0.2 to see changes in practice.

andreasvirkus commented 5 years ago

Thank you for a quick implementation 🙏 ! Also nice addition of locking the focus inside the modal. As this lib grows, you might consider something like micromodal for a base in the future, since it has great a11y support - https://micromodal.now.sh/

Quick question as well - wouldn't it be better to save the reference of newNode as you're inserting the modal and do a lookup for get okBtn and get cancelBtn via the modal's parent (instead of atm searching through the whole document, which could be very complex/large)?

f3oall commented 5 years ago

As far as I know document.getElementByID is pretty fast function. So I guess it's excess optimization of this code. For example check this answer on SO https://stackoverflow.com/questions/12514970/is-getelementbyid-efficient.

However, if you can make a benchmark and proof your suggestion with numbers, I will be glad to implement your idea.