svelte-modals icon indicating copy to clipboard operation
svelte-modals copied to clipboard

v2.0.0

Open mattjennings opened this issue 1 year ago • 11 comments

mattjennings avatar Aug 20 '22 21:08 mattjennings

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)

vercel[bot] avatar Aug 20 '22 21:08 vercel[bot]

@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.

mattjennings avatar Aug 20 '22 21:08 mattjennings

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.

MentalGear avatar Aug 21 '22 06:08 MentalGear

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.

c00 avatar Aug 24 '22 09:08 c00

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

MentalGear avatar Aug 24 '22 11:08 MentalGear

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.

MentalGear avatar Aug 24 '22 11:08 MentalGear

@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!

mattjennings avatar Aug 24 '22 13:08 mattjennings

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)

c00 avatar Sep 26 '22 00:09 c00

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 avatar Dec 29 '22 21:12 mattjennings

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:

c00 avatar Dec 31 '22 07:12 c00

Looking forward to get this released. 👍

michaltaberski avatar May 12 '23 08:05 michaltaberski