Closed ndarilek closed 2 years ago
Gotcha, I'll take care of that soon. That reminds me, do we really need to clean these up? In this case, the component is either visible or it isn't, and the events won't trigger if the component isn't visible. If the component gets destroyed, I'm assuming the elements to which the events are bound are no longer on the page, so the event handlers would be GC'd. Should I just remove that entirely? I copied that design decision from some of the other hooks even though I didn't fully understand the necessity.
Thanks.
Actually you are correct! We can simply kill the removeEventListener
's on destroyed because all the elements that have listeners exist within this.el
, and that element is being removed from the DOM, so there is nothing to clean up. The only time we need to be worried about cleaning up after ourselves in a hook is if we are binding outside of the hook element, such as to window, or another part of the DOM.
Got it. I'll remove the unnecessary cleanup when I return to this on Monday. Thanks for the feedback!
I think this should be good to go--please let me know if I missed anything. Thanks!
@ndarilek I couldn't push to this branch because it no longer exists on your GH, but I included you as a co-author in this commit: https://github.com/fly-apps/live_beats/commit/819d5ecc9850ff8f49013f3151213a4419ab44a2
I'll repeat the description here:
I built off your work to generalize focus wrapping, so we now have a <.focus_wrap>
component and hook that can be used to focus wrap any content. I also added focus
and focus_closest
JS commands that allow us to jump focus to next siblings when the user takes an action such as removing a row from a table. I've tested it out with the OS X screenreader and it works well based on my limited screen reader experience. I've deployed it to https://livebeats.fly.dev/chrismccord if you want to take it for a quick spin. If you find areas that need work, please start a new PR of master. Thanks!
@chrismccord The upload dialog now works well with the NVDA screen reader on Windows. Thanks.
routeUpdated
early to avoid redundant focus changes.