-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add capturing event listeners to the KeyboardShortcut library #3555
Conversation
|
Not sure at the time but it could have side effects. Like when we want to stop propagation for all listeners.
capturing will fail this scenario. |
Add capturing event listeners to the KeyboardShortcut library (cherry picked from commit 584183f)
🚀 Cherry-picked to staging in version: 1.0.68-2🚀
|
🚀 Deployed to production in version: 1.0.68-4🚀
|
🚀 Deployed to staging in version: 1.0.68-5🚀
|
🚀 Deployed to production in version: 1.0.73-3🚀
|
Shoutout to @HorusGoul for figuring this out. In the slack thread you made it sound like we should always have capturing event listeners, unless we specifically want to enable bubbling for certain events. I wasn't sure of the potential side-effects of preventing event bubbling by default, so I opted for the minimum change that resolves the current issue. Feel free to follow-up with any thoughts regarding other events which should not bubble.
Details
TBH I'm not 100% clear on why this fixes the linked issue, but shoutout to @HorusGoul for finding this! It works! 🎉
Fixed Issues
Fixes #3512
Tests / QA Steps (Web/Desktop only)
ReportActionCompose
is focused, hitCMD+K
.Esc
and the chat switcher should hide.Tested On
Screenshots
Web
Mobile Web
n/a
Desktop
iOS
n/a
Android
n/a