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

Use two-way binding #47

Closed
rgossiaux opened this issue Feb 2, 2022 · 5 comments
Closed

Use two-way binding #47

rgossiaux opened this issue Feb 2, 2022 · 5 comments
Labels
breaking change A breaking change

Comments

@rgossiaux
Copy link
Owner

When writing this initially I imitated the React API as closely as I could, to make it easy to adapt the Tailwind UI snippets.

However, a second guiding principle is that I want this to feel like a native Svelte library as much as possible. I believe that Tailwind follows this approach themselves with their React/Vue versions. And I think a native Svelte library should really just use bind here. Most convincing, though, is that the Vue library uses data binding with v-model on several components.

Need to figure out the right way to semver this... I'm still on 1.0.0-beta.x but I think I might have to bump to 2.0 just for this change, not sure yet.

@jeetrex
Copy link

jeetrex commented Feb 6, 2022

I concur, building it the svelte is the way forward.

@richarddavenport
Copy link

Agreed! We are actively using this. I'd love to help out too. I understand the desire to align with upstream, but I think some concessions would be greatly appreciated. How can I help?

@rgossiaux
Copy link
Owner Author

rgossiaux commented Feb 9, 2022

Agreed! We are actively using this. I'd love to help out too. I understand the desire to align with upstream, but I think some concessions would be greatly appreciated. How can I help?

From a technical perspective it's a simple change. But since you ask... one thing that would be helpful if you or anyone else have a little time is converting some of the unit tests to using svelte-inline-compile, the way this test here does:

Most of the tests were written before I discovered this library existed (it only had like 1 star on GH at the time), so I wrote my own component called TestRenderer to use my own custom syntax for testing multiple components together:

The syntax of this is basically [Component, Props, Children]

However I'd like to convert these tests to using svelte-inline-compile eventually, because the syntax is much better, and it can handle some things that aren't possible using my TestRenderer. One of those is bind:. I think in order to test bind: properly, a test method will want to use a store and then bind to it in the component.

The components that will be affected by this issue are Listbox, RadioGroup, Switch, and Tabs, so the tests there that involve changing the values will be the ones that are highest priority to convert. But I'd welcome any PRs to convert any of the tests from the old TestRenderer, if anyone feels so inclined.

(edit: this is done now)

@greendesertsnow
Copy link

It seems to me that this issue carries utmost importance. I keep going back and forth the documentation to remind myself how React implementation of a Svelte binding is written, namely on:change assign e.details to the value (??). And tbh, I use Vue versions of the snippets because of the import statements and all that, not React.

@rgossiaux rgossiaux mentioned this issue May 25, 2022
4 tasks
jangxyz added a commit to jangxyz/svelte-headlessui that referenced this issue Jun 8, 2022
temporary fix, until rgossiaux#47 and rgossiaux#80 is resolved.
ryoha000 pushed a commit to ryoha000/svelte-headlessui that referenced this issue Jun 3, 2023
temporary fix, until rgossiaux#47 and rgossiaux#80 is resolved.
@rgossiaux
Copy link
Owner Author

Version 2.0.0 is now published with this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change A breaking change
Projects
None yet
Development

No branches or pull requests

4 participants