-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Svelte migration (WIP) #448
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
Conversation
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a quick look over this it looks good to me. I do have some questions about how we're handling the getting/setting of values, out approach to loading components, the tooling we're using here (rollup, et al), and I also have a few accessibility concerns but I don't think this is the right place for those discussions/ changes.
We should try to get this in ASAP and keep it scoped tightly to just the svelte migration and we can have those other discussions and make any improvements in future, smaller, pull requests.
frontend-svelte/src/Interface.svelte
Outdated
{...input_component} | ||
{theme} | ||
value={input_values[i]} | ||
setValue={setValues.bind(this, i)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be more idiomatic as setValue={value => setValues(i, value)}
ae250ca
to
a15032b
Compare
Migrating React to Svelte.