search

svelteschool / svelte-forms

A simple form component that doesn't care about what you put in it. It just works.
137 stars 5 forks source link

fix: 🐛 Fix multiple select bug #4

Closed rodgco closed 4 years ago

rodgco commented 4 years ago

Hi Kevin, saw your presentation at Svelte Society Day 2020 and gave a try to svelte-forms. Unfortunately there was a problem with the multiple-select input that cleared the selection after the second selection. The problem was that deserialize was not handling that type of input.

Unfortunately again, I don't handle code with short variable names very well, so I refactored all you serialization code (hopefully for the best).

Thank you for this one, it's a very clever and simple solution!

kevmodrome commented 4 years ago

Hey! Thanks for the PR, do you have a REPL show-casing the bug?

rodgco commented 4 years ago

Link to REPL with the issue.

I've also modified the two-way data binding button.

kevmodrome commented 4 years ago

Sorry this took a couple of days to look into. I see the issue here. I think it's fine if we re-factor this as well to be a bit more readable.

I do think there should be some tests added to support these cases. With that added we can merge this! 🚀

rodgco commented 4 years ago

Hi Kevin, sorry for the delay. Managing different types of work during this period seems more complex. Still working on the tests, struggling a bit here... not sure if it's a problem in the code or with the test API (trying to use userEvent.selectOptions). As soon as I have news I'll post it here.

rodgco commented 4 years ago

Hi Kevin, just I explained before couldn't find a way to trigger the update on multi-select testing. So I used a "workaround" to test this case. Comments in code.

As a bonus added tests for reactivity.

kevmodrome commented 4 years ago

Looks good! Very thankful for the PR! Sorry for the somewhat slow response.