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

fix binding group with each #4868

Merged

Conversation

tanhauhau
Copy link
Member

Fixes #3243

This builds on top of #4861, therefore contains the commit of the PR

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
  • This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
  • Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

  • Run the tests tests with npm test or yarn test)

@tanhauhau tanhauhau force-pushed the tanhauhau/binding-group-with-each branch from 99975b5 to c506f2e Compare May 20, 2020 10:25
@tanhauhau tanhauhau added the each label May 31, 2020
@Conduitry
Copy link
Member

@tanhauhau Can you take a look at the merge conflicts here as well please? I don't think these are all caused by bindingGroup/binding_group stuff either.

@tanhauhau tanhauhau force-pushed the tanhauhau/binding-group-with-each branch from c506f2e to d723a3f Compare June 9, 2020 01:07
@tanhauhau
Copy link
Member Author

@Conduitry conflicts resolved.

@Conduitry
Copy link
Member

Hm. Can you explain more precisely what this is intended to fix? I'm trying out this branch against the various examples given in REPLs in #3243 and I'm not sure what I'm supposed to be seeing. I see different behavior than I am in the version linked to in the REPLs, but in several cases nothing seems to be happening now when clicking on different checkboxes.

@tanhauhau
Copy link
Member Author

tanhauhau commented Jun 9, 2020

so previously sharing the same binding_group is based on the variable name, eg:

<input bind:group={a} value="a">
<input bind:group={a} value="b">
<input bind:group={b} value="c">
<input bind:group={b} value="d">

{a} {b}

"a", "b" share the same binding group a, "c", "d" share the same binding group b.

however in a {#each}, this may break

{#each array as item}
    <input bind:group={item} value="a">
    <input bind:group={item} value="b">
    {item}
{/each}

svelte repl

although the bind:group uses the same variable name, item, they should be bound to a different binding group.

the current behavior is that they all share the same binding group.
So you would notice after selecting all the checkbox in the first each item, selecting any checkbox in the 2nd each item will select all the checkboxes within them.

The fix is to figure out if the variable name is under a each context, and treat each {#each} item bind:group={item} binds to a different binding group

@Conduitry Conduitry merged commit 38de3b2 into sveltejs:master Jun 10, 2020
@tanhauhau tanhauhau deleted the tanhauhau/binding-group-with-each branch June 10, 2020 21:47
taylorzane pushed a commit to taylorzane/svelte that referenced this pull request Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array binding not works properly.
2 participants