search

flekschas / svelte-simple-modal

A simple, small, and content-agnostic modal for Svelte v3 and v4
https://svelte.dev/repl/b95ce66b0ef34064a34afc5c0249f313
MIT License
422 stars 30 forks source link

Minor code improvement #68

Closed justingolden21 closed 2 years ago

justingolden21 commented 2 years ago

Hi, I'm wondering why on line 334 you ad tabbable.length since you're doing % tabbable.length the next line anyway, it shouldn't matter, right?

image

https://github.com/flekschas/svelte-simple-modal/blob/master/src/Modal.svelte#L334

justingolden21 commented 2 years ago

Oh is it to make it a positive number in the case that index was -1 before and it's shift tab?

flekschas commented 2 years ago

Thanks for asking. The code isn't too obvious haha.

Yeah, -1 % 5 === -1 but we want to move the focus to a tabbable element inside the modal. The shiftKey modification just inverses the sequence of focus elements. So in other words, when you press SHIFT and then press TAB the last tabbable element is focused first, then the second last, and so on.

But really, tabbable.length is only added to ensure that index is in [0, tabbable.length[ (all numbers from 0 to tabbable.length - 1). The modulo operation only ensure this for positive numbers. Hence we need to make sure that index is positive before we apply the modulo.

I hope this explains it.

justingolden21 commented 2 years ago

Yeah that makes sense; thanks for explaining!

I was just looking at the source code because I'm having trouble getting this working in my own modal haha. Your code is very well written 😄