Skip to content
This repository has been archived by the owner on Nov 22, 2024. It is now read-only.

Modals #21

Merged
merged 3 commits into from
May 18, 2020
Merged

Modals #21

merged 3 commits into from
May 18, 2020

Conversation

illright
Copy link
Owner

Okay, this one's particularly crazy. I've been trying to meet the following requirements:

  • it should be possible to create a modal anywhere under the ModalOverlay component
  • it should preserve state between open/close, i.e. not be recreated on every open
  • it should be controlled by a simple boolean variable
  • it should be reactive to prop changes

From the looks of it, it works.

@illright illright requested a review from aabounegm May 17, 2020 21:49
@github-actions
Copy link

Demo available on https://illright.github.io/attractions/21/

Copy link
Collaborator

@aabounegm aabounegm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you're saying the <ModalOverlay> has to wrap everything?
Do we even need it when there is <svelte:body> (to add modals to the root) and <script context="module"> (to handle the registration and shared data)?

export let open = false;

/* The component to use in creating a modal. */
export let component;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like the idea of taking a component as a prop instead of a slot. I see only 2 ways around it:
using $$props.$$slots, which works but isn't part of the official API (although it is used in some libraries. Check out sveltejs/svelte#2106 (comment) for an example and the rest of the discussion) but would make more sense for the user; it will console.warn that the component doesn't expect a slot, though. The other way would be to wrap a slot in a meaningless div just to have something to bind:this to, and use that as the component. This seems like the more resilient way, although it introduces a useless element

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me neither, but that is the only way we can have the <Modal> instances anywhere in the descendants of the <ModalOverlay>, which was a first requirement. Otherwise it would create this slotted component right where it is placed.
Some might argue something like a portal could remedy that, but no such thing exists in Svelte as of now, and doing it manually with appendChild is a sure way for disaster.

Not only is $$props.$$slots internal API, the use of $$props themselves is discouraged by Svelte.

As a final note, given the experience of writing a shit ton of modals in the View Project page of Innopoints, I can assure you that you always want the modal window to be a separate component. Simply because it contains some sort of logic that is best isolated away.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But don't you think that this way is much more limiting than using $$props.$$slots (which was even sorta suggested by a Svelte member: sveltejs/rfcs#15 (comment)) or using a dummy wrapper with bind:this? You are only passing props (in a way that looks weird, tbh), which doesn't allow listening for events or adding directives

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But don't you think that this way is much more limiting than using $$props.$$slots...

Yes, it is more limiting. But again, that's okay because you want the modal created in a different place, in fact. And because most modals require a component of their own for convenience anyway. This is a limitation that we cannot get around with, for if we go with $$props.$$slots, we will end up creating the component further down the element tree, and our only option would be to move it.

You are only passing props (in a way that looks weird, tbh), which doesn't allow listening for events or adding directives

Not exactly. Remember the events action? Here's your way to listen to events. As for directives, you cannot add them to a custom component anyway (I think), and even if you'd want to, you could do that in the component itself, to one of its elements, and then pass that component here.

@illright
Copy link
Owner Author

So you're saying the has to wrap everything?

Not necessarily everything, the part that will have this gray overlay and a modal in the center. Though in most cases, it will, in fact, wrap everything.

<svelte:body> is not exactly to add modals to the root. In fact, it cannot even have children, unlike <svelte:head>. <svelte:body> is only meant for attaching event handlers to the body element.

<script context="module"> would mean it's impossible to have several parallel ModalOverlays, if one desires. The approach taken with the context is such that only the descendants of the ModalOverlay are aware of anything, and the rest of the tree can be completely independent, and even have another ModalOverlay somewhere. I just think this independence makes it clearer.

@aabounegm
Copy link
Collaborator

The point in using <script context="module"> is to not need ModalOverlay at all. Who even needs multiple ones?
Also, even by wrapping just one component, the overlay covered the entire screen

Copy link
Collaborator

@aabounegm aabounegm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think it's a bit complicated, but whatevs)

@illright illright merged commit e98f06c into develop May 18, 2020
@illright illright deleted the feature/modals branch May 18, 2020 20:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants