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

chore(docs): note about reactive options #836

Merged
merged 1 commit into from
Apr 28, 2024

Conversation

meirroth
Copy link
Contributor

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.

@davidjerleke davidjerleke added the documentation Improvements or additions to documentation label Apr 22, 2024
@meirroth
Copy link
Contributor Author

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

<script setup>
  import emblaCarouselVue from 'embla-carousel-vue'

  const options = ref({ loop: true })

  const [emblaRef] = emblaCarouselVue(options)

  // ...
</script>

@meirroth
Copy link
Contributor Author

meirroth commented Apr 23, 2024

@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
Copy link
Owner

davidjerleke commented Apr 24, 2024

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

meirroth commented Apr 25, 2024

@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
Copy link
Owner

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
Copy link
Contributor Author

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

Co-authored-by: David Jerleke <david.jerleke@gmail.com>
Co-authored-by: Meir Roth <12494197+meirroth@users.noreply.github.com>
@davidjerleke davidjerleke merged commit 5ae8348 into davidjerleke:master Apr 28, 2024
@davidjerleke davidjerleke added the resolved This issue is resolved label Apr 28, 2024
@davidjerleke
Copy link
Owner

@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 meirroth deleted the patch-1 branch April 28, 2024 12:53
@meirroth
Copy link
Contributor Author

@davidjerleke Awesome, thank you!

@meirroth
Copy link
Contributor Author

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
Copy link
Owner

@meirroth yes! And here of course.

@meirroth
Copy link
Contributor Author

@davidjerleke Great work! Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation resolved This issue is resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants