mattjennings / svelte-modals

A simple, flexible, zero-dependency modal stack manager for Svelte.
https://svelte-modals.mattjennings.io
154 stars 8 forks source link

v2.0.0 #22

Closed mattjennings closed 1 month ago

vercel[bot] commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

This is awesome, thank you - I'll try to check it out in the next days!

On 20 Aug 2022, at 23:55, Matt Jennings @.***> wrote:

@MentalGear https://github.com/MentalGear @c00 https://github.com/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 @.*** and the latest docs can be seen here <x-msg://4/svelte-modals.next.mattjennin.gs>.

— Reply to this email directly, view it on GitHub https://github.com/mattjennings/svelte-modals/pull/22#issuecomment-1221415580, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAVUVGYREHVVTYJGUOURSUTV2FH5JANCNFSM57D2TDSA. You are receiving this because you were mentioned.

c00 commented 2 years ago

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 commented 2 years ago

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

MentalGear commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 1 year ago

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>
c00 commented 1 year ago

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 :tada:

michaltaberski commented 1 year ago

Looking forward to get this released. 👍

mattjennings commented 1 month ago

Given the release of Svelte 5, I'm planning on doing a 2.0.0 that does not include these changes but upgrades to Svelte 5 APIs. This means event handlers are now callback props, and that makes most of the changes here unnecessary. But I also want to address a lot of the outstanding issues + the issues leading to these changes in subsequent v2 releases, so I'm going to refresh my memory on all the discussion here and make sure I address them.

Again my apologies on the lack of activity here.