davidjerleke / embla-carousel

A lightweight carousel library with fluid motion and great swipe precision.
https://www.embla-carousel.com
MIT License
5.37k stars 164 forks source link

chore(docs): note about reactive options #836

Closed meirroth closed 2 months ago

meirroth commented 2 months ago

This PR adds a note mentioning that in some Embla wrappers it's possible to pass reactive options and Embla will automatically reinitialize when they change.

meirroth commented 2 months ago

@davidjerleke Should we update the Vue example (and others) to use a ref, to demonstrate reactivity?

```html highlight={4}
meirroth commented 2 months ago

@davidjerleke Reading the docs again, and looking at the source code I think we should update the the Vue guide to mention:

  1. Options and plugin reactivity feature. Perhaps with an example showing passing options as a ref.
  2. Auto destroy before unmount.
  3. Update the API example to use onMounted instead of watchEffect.

Let me know what you think.

BTW, is it necessary to run .off on subscribed events before unmount or does destroy handle that?

davidjerleke commented 2 months ago

@meirroth thanks for your suggestions.

  1. Options and plugin reactivity feature. Perhaps with an example showing passing options as a ref.

Maybe adding a heading below Global options and name it Reactive options? That section could demonstrate a code example off how to work with reactive options for all reactive Embla wrappers: React, Vue, Solid and Svelte.

  1. Auto destroy before unmount.

I agree. We should add this information on all get started pages get-started/<embla-package>. Maybe it makes sense to add a heading after Adding plugins called Destroying a carousel?

  1. Update the API example to use onMounted instead of watchEffect.

Yes, I agree. Unfortunately the watchEffect examples are sprinkled out so we need to find these places:

Events

Methods

Plugins

BTW, is it necessary to run .off on subscribed events before unmount or does destroy handle that?

destroy doesn't handle that but I think it should. To achieve this it has to be added after this line. We also need to add a clear method here like so:

function clear(): void {
  listeners = {}
}

Best, David

meirroth commented 2 months ago

@davidjerleke I'll work on all of this once I get the repo running localy.

destroy doesn't handle that but I think it should. To achieve this it has to be added after this line. We also need to add a clear method here like so:

function clear(): void {
  listeners = {}
}

Can you please clarify what you meant by "it has to be added after this line."

  1. What exactly should be added after that line?
  2. Are we updating the core package or only in the vue package?
  3. Wouldn't clearing event listeners be handled within the destroy method?
  4. I'm guessing we'll handle removing event listeners by calling .off on all subscribed listeners, or would you do it differently?
meirroth commented 2 months ago

Also, trying to understand if engine.eventStore.clear() clears event listeners, if so, it looks like it's already handled by deActivate which destory calls.

meirroth commented 2 months ago

@davidjerleke Done 1 and 3.

Thanks a ton!

For 2, I'm just thinking whether instead of adding a new section like you suggested for mentioning that the framework wrapper auto destroys the carousel we should add an opening line at the top of the page, above "Start by installing the Embla Carousel...", something like this:

Vue

Embla Carousel provides a wrapper for Vue that ensures seamless integration of the carousel into your Vue project and automatic cleanup on component unmount.

Start by installing the Embla Carousel npm package and add it to your dependencies.

Thoughts?

Very good solution! We should add and automatic cleanup on component unmount to all other wrappers: React, Solid, Svelte.

For 3, I'm only familiar enough to provide examples for Vanilla and Vue, can you please help me with the other frameworks?

Sure! I will do that 👍.

Another thing, sorry for changing my mind but I think we should name the heading Reactive options --> Changing options because we include vanilla JS there too.

davidjerleke commented 2 months ago

Can you please clarify what you meant by "it has to be added after this line."

  1. What exactly should be added after that line?

I can look into this as it's a bit hard to exmplain.

  1. Are we updating the core package or only in the vue package?

The core package 🙂.

  1. Wouldn't clearing event listeners be handled within the destroy method?

Yes they should.

  1. I'm guessing we'll handle removing event listeners by calling .off on all subscribed listeners, or would you do it differently?

We can under the lifecycle of a carousel add (.on) and remove (.off) listeners but they should be cleared when the carousel is destroyed. This isn't a big problem because I don't get bug reports about it but still we should do the housekeeping.

meirroth commented 2 months ago

@davidjerleke Thank you for your guidance! I've update the title, and added an introduction line to all wrappers.

What's left for this PR is the reactive examples.


We can continue to discuss removing event listeners here for continuity sake, but I think I'll open a new PR if any changes needed.

Surfacing what I wrote earlier: does mediaHandlers.clear() or deActivate() --> engine.eventStore.clear() called by the destory() method handle clearing event listeners? If so, I think we're good as is, and there's no need to manually run .off on subscribed events before unmount, as I initially asked.

davidjerleke commented 2 months ago

@meirroth fantastic work here. I added the missing examples. Now we only have to update the api/plugins page because plugins are reactive too 😅. But let's do that in a separate PR.

About this:

Surfacing what I wrote earlier: does mediaHandlers.clear() or deActivate() --> engine.eventStore.clear() called by the destory() method handle clearing event listeners? If so, I think we're good as is, and there's no need to manually run .off on subscribed events before unmount, as I initially asked.

I will create a PR to show you what I mean. Stay tuned 👍.

meirroth commented 2 months ago

@davidjerleke Awesome, thank you!

meirroth commented 1 month ago

Surfacing what I wrote earlier: does mediaHandlers.clear() or deActivate() --> engine.eventStore.clear() called by the destory() method handle clearing event listeners? If so, I think we're good as is, and there's no need to manually run .off on subscribed events before unmount, as I initially asked.

I will create a PR to show you what I mean. Stay tuned 👍.

@davidjerleke Was this addressed here? 😏 https://github.com/davidjerleke/embla-carousel/pull/871/files#diff-cb6e53e6d149a1a6784922356eaff9e7a5f0a43a0d30203fec752c39b60aeaf0R155

davidjerleke commented 1 month ago

@meirroth yes! And here of course.

meirroth commented 1 month ago

@davidjerleke Great work! Thank you