Do some cleanup on some internal functions #380
Conversation
| // This is an escape hatch in case there are nested dialogs, | ||
| // so the keypresses are only reacted to for the most recent one | ||
| if (!this.$el.contains(document.activeElement)) return | ||
|
|
There was a problem hiding this comment.
🔍 This is no longer necessary if we bind this event listener specifically to this.%el, instead of to the document
There was a problem hiding this comment.
Can you double check with the nested dialogs fixture that it still works okay?
There was a problem hiding this comment.
It does seem to work ok. I did some manual testing and Cypress is still happy.
| // If the dialog is shown and the ESC key is pressed, prevent any further | ||
| // effects from the ESC key and hide the dialog, unless its role is | ||
| // `alertdialog`, which should be modal | ||
| if ( | ||
| this.shown && |
There was a problem hiding this comment.
🔍 Checking for this.shown is redundant here because this listener is bound and unbound when we show and hide, right?
There was a problem hiding this comment.
Right, I think that makes sense yes.
| @@ -209,12 +204,8 @@ export default class A11yDialog { | |||
| * See: https://github.com/KittyGiraudel/a11y-dialog/issues/177 | |||
| */ | |||
| private maintainFocus = (event: FocusEvent) => { | |||
| if (!this.shown) return | |||
There was a problem hiding this comment.
🔍 Same as bindKeypress – checking for shown is redundant, right?
| @@ -98,7 +98,7 @@ export default class A11yDialog { | |||
| // stays trapped inside the dialog while open, and start listening for some | |||
| // specific key presses (TAB and ESC) | |||
| document.body.addEventListener('focus', this.maintainFocus, true) | |||
| document.addEventListener('keydown', this.bindKeypress) | |||
There was a problem hiding this comment.
🔍 Listening for this event during the capturing phase is just a little 🤏🏻 faster, since the capturing phase runs first
There was a problem hiding this comment.
Heh that’s fair enough. I guess we really only need to listen to TAB keys in the container itself. It shouldn’t break anything.
mxmason
force-pushed
the
ej/chore/clean-up-internals
branch
from
44a6dca
to
9107fdf
Compare
Summary
Just trying to shave mere bytes off of the amount of code we ship.