Skip to content

Reactive selected prop #136

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

Closed
cbenz opened this issue Oct 21, 2022 · 5 comments · Fixed by #138
Closed

Reactive selected prop #136

cbenz opened this issue Oct 21, 2022 · 5 comments · Fixed by #138
Labels
bug Something isn't working dx Developer experience tests Test all the things

Comments

@cbenz
Copy link

cbenz commented Oct 21, 2022

I'm surprised to read in the docs that the selected prop is static ("[...] or passed as prop to set pre-selected options that will already be populated when the component mounts before any user interaction").

In my case, the selected options depend on the external context, the URL params, and when I go back/forward the MultiSelect has to update in consequence.

This worked as I expect with version 6.1.0 but since version 7.1.0, the selected prop has become static (it does not take new values into account).

Is this intended behavior?

As a workaround, I did use:

{#key selected}
  <MultiSelect {options} {selected} />
{/key}

Here is a quick-made REPL to demonstrate this: https://svelte.dev/repl/153f9049a9b849aa9fcc95117061a867?version=3.52.0

@janosh
Copy link
Owner

janosh commented Oct 22, 2022

Thanks for bringing this up and for the nice REPL repro. This is not intended behavior. I'm pretty sure this is a consequence of #123 which added an extra layer in between the selected and what the component actually renders in order to have selected be a single option (instead of a length-1 array) if maxSelect={1}. Can't please everyone, I guess, eh @davipon? 😄

Seriously though, it's too bad I haven't found a good way of testing 2-way binding yet which would have prevented this regression.

@janosh janosh added bug Something isn't working tests Test all the things dx Developer experience labels Oct 22, 2022
@janosh janosh changed the title Reactive selected prop Reactive selected prop Oct 22, 2022
@wd-David
Copy link
Contributor

Thanks for bringing this up and for the nice REPL repro. This is not intended behavior. I'm pretty sure this is a consequence of #123 which added an extra layer in between the selected and what the component actually renders in order to have selected be a single option (instead of a length-1 array) if maxSelect={1}. Can't please everyone, I guess, eh @davipon? 😄

Yes, this is the consequence of #123. I was also preparing a repro of this 😊.

Seriously though, it's too bad I haven't found a good way of testing 2-way binding yet which would have prevented this regression.

Not sure if my svelte-component-test-recipes could help.
Even though I don't like the svelte-htm locked-in approach, it works anyway.

@janosh
Copy link
Owner

janosh commented Oct 22, 2022

Yeah, I was just rereading your two-way binding recipe. Very cool that it works but it would require two new dependencies. I might just go with a wrapper component that dispatches events for testing whenever the two-way bound variable changes.

@janosh
Copy link
Owner

janosh commented Oct 22, 2022

Maybe the best solution for regaining 2-way binding for selected is to create a new prop value which should only be used if maxSelect == 1 in which case it is selected[0]. If maxSelect != 1 it would be null to signal that this shouldn't generally be used. We could also set value = selected if `maxSelect != 1, not sure which is better.

The only problem with this is that you can't control value with 2-way binding (same as selected now), you'd have to use selected for that, even if maxSelect == 1. I don't currently see a way around that.

@janosh
Copy link
Owner

janosh commented Oct 22, 2022

Released in v8.0.0. I confirmed that installing

import MultiSelect from "[email protected]"

in your REPL fixes the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dx Developer experience tests Test all the things
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants