Fixes #3391 - Add ability to close modals with Esc key #3446
Conversation
| modalTriggers.forEach(function (trigger) { | ||
| trigger.addEventListener("click", function (e) { | ||
| e.preventDefault(); | ||
| sendAnalyticsEvent("whatIsCompat", "click"); |
There was a problem hiding this comment.
It turns out we sent this event when opening the "learn more" modal as well. 🤷
miketaylr
force-pushed
the
issues/3397/1
branch
from
f87ef78 to
2711b33
Compare
| // transform trigger into camelCase and send as our event | ||
| popupTrigger.replace(/-([a-z])/g, function (match) { | ||
| return match[1].toUpperCase(); | ||
| }), |
There was a problem hiding this comment.
Since analytics goal tracking for this step is set up as whatIsCompat, we probably need to keep it that way. It's possible to change the label, but then previous weeks data will still have the old label and won't be reflected in the statistics :(
We can probably change data-popup-trigger="what-is-webcompat" to data-popup-trigger="what-is-compat" (and same with data-popup-modal)
There was a problem hiding this comment.
oh yeah! i didn't even notice the difference there, thanks :)
|
OK, just re-tested after changing the string. Seems to be working locally. r? @ksy36 |
|
(hm, test failure -- maybe hold off on review) |
|
lol I pressed rerun it before I saw your comment |
miketaylr
force-pushed
the
issues/3397/1
branch
from
ad0b211 to
3204668
Compare
|
OK, tests seem happy, and merge conflict resolved. :) |
|
Thanks! |
This lets users close the modal with Esc, and adds a test.
r? @ksy36