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

[Feat]: Adding updated onemblaInit naming for Svelte 5 #992

Merged
merged 3 commits into from
Sep 11, 2024

Conversation

S1r-Lanzelot
Copy link
Contributor

Per the new naming conventions of event handlers: https://svelte-5-preview.vercel.app/docs/event-handlers

This is necessary due to deprecation moving forward and because Mixing old and new syntaxes for event handling is not allowed. (mixed_event_handler_syntaxes) sveltejs/svelte#11295

leaving on:emblaInit because it will be backward compatible.

Thanks

@davidjerleke davidjerleke changed the title adding updated onemblaInit naming for svelte 5 [Feat]: Adding updated onemblaInit naming for Svelte 5 Sep 6, 2024
@davidjerleke davidjerleke added feature request New feature or request svelte Issue is related to Svelte labels Sep 6, 2024
@davidjerleke
Copy link
Owner

davidjerleke commented Sep 9, 2024

Hi @S1r-Lanzelot,

Thanks for your contribution 👍. I think this is a great initiative.

leaving on:emblaInit because it will be backward compatible.

Good thinking. Even though it's deprecated, on:emblaInit will continue to work with Svelte 5 so devs won't be forced to upgrade their Svelte version to 5 just to use the latest version of Embla.

Have you tested the carousel with Svelte 5? Does it work? If yes, I think we can include this in your PR:

Would you mind adding that to this PR?

Cheers!
David

@S1r-Lanzelot
Copy link
Contributor Author

S1r-Lanzelot commented Sep 9, 2024

Yes I am using it now but to be honest not all the features. Nevertheless, since we are only concerned about ActionResult then my limited use should be sufficient. I can preemptively include the 5.0.0 so embla-carousel is ready when Svelte 5.0 is finally released. In the meantime I recommend using an override. Sounds good?

Let me bang on my use for another day and then I expand the peer dep in the PR 👍

"overrides": {
    "embla-carousel-svelte": {
      "svelte": "^5.0.0-next.1"
    }
  }

@S1r-Lanzelot
Copy link
Contributor Author

Additionally, I have updated the docs. I was back and forth with which syntax to include as the default, let me know if this is "jumping the gun," I can default it to Svelte 4 syntax if preferred:
image

@davidjerleke
Copy link
Owner

@S1r-Lanzelot one thought. Should we make it lowercase for Svelte 5? Like so: onemblaInit --> onemblainit? Because it seems like that's convention for events but I'm not a seasoned Svelte dev by no means so let me know what you think.

Additionally, I have updated the docs. I was back and forth with which syntax to include as the default, let me know if this
is "jumping the gun," I can default it to Svelte 4 syntax if preferred:

@S1r-Lanzelot thanks. The admonition with the backward compatibility is a great idea I think 👍.

There are additional on:emblaInit code examples in the docs so if we're to change in one place I think we should replace all and also add the same admonition on all places. For example, the event handler is used here:

Methods

Events

Plugins

Thanks for your efforts!

@S1r-Lanzelot
Copy link
Contributor Author

S1r-Lanzelot commented Sep 10, 2024

@davidjerleke - thanks for pointing out the additional references in the docs, I'll get these.

Should we make it lowercase for Svelte 5? Like so: onemblaInit --> onemblainit? Because it seems like that's convention for events but I'm not a seasoned Svelte dev by no means so let me know what you think.

It's a good question, especially now that onemblaInit isn't [visibly] following a clear naming convention. It probably should be all lowercase just to keep things consistent and because in some cases there is good reason for it.

However, I vote to leave it only because of the other changes I think it would require since events are case sensitive and a change would require an update here I believe. Although to be honest the change is quite minimal with simply dispatching two events instead of one (as to not introduce a breaking change), but not sure it's worth it in the end...

Let me know your thoughts, thanks.

@davidjerleke
Copy link
Owner

davidjerleke commented Sep 11, 2024

It's a good question, especially now that onemblaInit isn't [visibly] following a clear naming convention. It probably should be all lowercase just to keep things consistent and because in some cases there is good reason for it.

However, I vote to leave it only because of the other changes I think it would require since events are case sensitive and a change would require an update here I believe. Although to be honest the change is quite minimal with simply dispatching two events instead of one (as to not introduce a breaking change), but not sure it's worth it in the end...

@S1r-Lanzelot good point 👍. Of course, that's a breaking change so we need to wait until the next major release. I've added the task to the roadmap so we don't forget about it:

Left to do

  • Update the additional references of on:emblaInit in the docs.
  • Add Svelte 5 to package.json: "^3.49.0 || ^4.0.0" --> "^3.49.0 || ^4.0.0 || ^5.0.0".

@S1r-Lanzelot
Copy link
Contributor Author

@davidjerleke thanks for all the feedback! Let me know if it needs anymore tweaking but I think we're good to go.

@davidjerleke
Copy link
Owner

@S1r-Lanzelot one last question: Do you know if this "^3.49.0 || ^4.0.0 || ^5.0.0" will accept Svelte 5 next versions as well (5.0.0-next.1 etc.)?

@S1r-Lanzelot
Copy link
Contributor Author

S1r-Lanzelot commented Sep 11, 2024

No it won't unfortunately, it will still require an override in people's reference until Svelte 5 is officially released, Flowbite svelte does a similar approach and if people want to be on the bleeding edge then they can just override.

"overrides": {
    "embla-carousel-svelte": {
      "svelte": "^5.0.0-next.1"
    }
  }

@davidjerleke
Copy link
Owner

@S1r-Lanzelot thanks for your contributions! We're good to go then 👍.

@S1r-Lanzelot
Copy link
Contributor Author

Sweet - thanks for the library!

@davidjerleke davidjerleke changed the title [Feat]: Adding updated onemblaInit naming for Svelte 5 [Feat]: Adding updated onemblaInit naming for Svelte 5 Sep 11, 2024
@davidjerleke davidjerleke added the resolved This issue is resolved label Sep 11, 2024
@davidjerleke davidjerleke merged commit 1d3964d into davidjerleke:master Sep 11, 2024
davidjerleke added a commit that referenced this pull request Sep 11, 2024
[Feat]: Adding updated `onemblaInit` naming for Svelte 5
davidjerleke added a commit that referenced this pull request Sep 11, 2024
[Feat]: Adding updated `onemblaInit` naming for Svelte 5
@davidjerleke
Copy link
Owner

@S1r-Lanzelot I just released Svelte 5 support along with the documentation changes in v8.3.0. Thanks for your efforts 🎉!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request resolved This issue is resolved svelte Issue is related to Svelte
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants