Unnecessary `addEventListener`s

[0djek]

In Modal.svelte< there are code to add event listeners, instead of binding to the object directly. Those event listeners are also not closed after the modal is not needed anymore. Is there a reason why it was done like that?

[aryadaroui]

Yes, so the modals are actually always in the DOM. when they're closed we set their opacity to 0 and disable pointer events on them. Because they're always in the DOM, and because those even listeners only set their state to closed with no side effects, we don't need to remove the event listeners on closing.

I did it this way because it was simpler than using svelte {#if}s in the markup, and trying to animate the opening and closing. Maybe my initial approach was wrong though.

I'm not sure what you mean by "binding to object directly". Is there a bind:* command I'm unaware of? Or do you maybe mean using an event dispatcher?

[0djek]

I don't remember correctly how it's called, but it's possible in svelte to transition objects for when they appear/disappear in the DOM. It would be possible to then use the {#if} inside the modal component itself. This would help reduce the clutter and make the code a bit more clear, we also wouldn't need to disable any events. More on these: Svelte tutorial for transitions

My bad, you can attach event listeners to the DOM element using on:{event-name}. It also works with custom events created using createEventDispatcher and you can annotate it using typescript, so other components, that use it, get correct type information. This is also revelant inside Toggle.svelte component (I think this is how it's called), where you accept function instead of dispatching event and allowing user to handle it more correctly. More on these: Svelte tutorial for events

If you want, I could create PR with these changes for some component so that it would be more clear by what I mean.

[aryadaroui]

Ohhh yes. Two great points.

I think I might indeed be able to use svelte transitions to make the same effect if i combine them.

And i see what you mean now. Yes that should probably be done for the non-window event listeners. I agree.

I'll add these to the todos

[aryadaroui]

Note to self: this is applicable for all components that appear and disappear, like modal, filter, and goto