Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v2.0.0 #22

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from
Draft

v2.0.0 #22

wants to merge 21 commits into from

Conversation

mattjennings
Copy link
Owner

No description provided.

@vercel
Copy link

vercel bot commented Aug 20, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
svelte-modals ✅ Ready (Inspect) Visit Preview Sep 23, 2022 at 0:11AM (UTC)

@mattjennings
Copy link
Owner Author

mattjennings commented Aug 20, 2022

@MentalGear @c00 this is pretty much ready to go, but I'd love if you could try it and share any feedback you might have as it largely focuses both of your raised issues. Otherwise, this will probably get released sometime next week!

It's available as svelte-modals@next and the latest docs can be seen here.

@MentalGear
Copy link

MentalGear commented Aug 21, 2022 via email

@c00
Copy link

c00 commented Aug 24, 2022

I like this a lot! I'm super happy with the way we await results. The event dispatching also seems well done!

One of the things that I found a bit confusing at first is that it's not obvious that the isOpen and close props get added by the modal service. Unless you look at the documentation (or just know), there's no way to infer that these are props are given to you. It's definitely not a huge problem. It's just slightly magical. It's worth considering changing it to a context in the future, so that the close, isOpen and whatever else, just get added to a context using setContext(). That way all you'd need was the type of the context, and Typescript would know which props / functions / etc. were available from the modal service.

@MentalGear
Copy link

Second the notion on modal props. v.2 would be a great opportunity to hide them in context!

@MentalGear
Copy link

Much appreciate the new "createModalEventDispatcher", which usage is also much more idiomatic for svelte instead of the previous callback. This could become the definitive svelte modals lib!
PS: On the guide page, the example with "Delete Important Data": When the second modal is opened, the backdrop fades in and out.

@mattjennings
Copy link
Owner Author

mattjennings commented Aug 24, 2022

@c00 Re: props I share the same concern. I did give a context-like approach some thought initially but felt it would be too boilerplate-y. But I am willing to give it more thought. This was roughly my idea:

<script>
import { register } from 'svelte-modals'

const $modal = register({
   // probably would be renamed to preventClose or something
   onBeforeClose: () => {},

   /* ... other eventual config */
})

// would provide
$modal.isOpen
$modal.close()
$modal.dispatch()

</script>}

alternatively

<script>
import { register } from 'svelte-modals'

const { isOpen, close, dispatch }= register({ ... })

// potential foot-gun. if someone didn't know isOpen was a store, it would always be truthy
$isOpen

// but you wouldn't need to access everything else through $modal
close()
dispatch()

</script>}

What would you think of either approach? This actually would not be using context, hence why getContext() isn't used. This register method would provide functions that are scoped to that specific modal in the stack, i.e the close and dispatch methods.

@MentalGear I'll take a look at the backdrop fading, thanks!

I'm glad you are both happy with the events/returned value solutions though!

@c00
Copy link

c00 commented Sep 26, 2022

I am unfamiliar with the register call so I'm not sure how that works, but for my understanding is it enough to assume it works like getContext, but scoped to the active modal?

const { isOpen, close, dispatch }= register({ ... })

This approach is my favorite. It also seems to be most svelte-ish. Assuming it's typed it correctly, the IDE will know what the properties are, so it shouldn't be too much of a problem. (Also, every example of a modal on the svelte REPL uses a store for its isOpen-equivalent, so I think it's okay)

@mattjennings
Copy link
Owner Author

mattjennings commented Dec 29, 2022

Apologies for the lack of activity on this. It's been a busy end of the year for myself and I've had very little time or energy to devote to OSS in general. I can't make any promises on timelines but I want to give this API some serious thought before committing to it.

At this point, I think it's most likely to be this:

<script>
import { register } from 'svelte-modals'

const { isOpen, close, dispatch }= register({ ... })
</script>

@mattjennings mattjennings marked this pull request as draft December 29, 2022 21:33
@c00
Copy link

c00 commented Dec 31, 2022

That approach works well for me.

It's been a busy end of the year

I hope you're doing well, man. Have a great end of the year 🎉

@michaltaberski
Copy link

Looking forward to get this released. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants