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

[choice-group] check multipleChoice change detection #1028

Closed
tlouisse opened this issue Oct 15, 2020 · 1 comment
Closed

[choice-group] check multipleChoice change detection #1028

tlouisse opened this issue Oct 15, 2020 · 1 comment

Comments

@tlouisse
Copy link
Member

Expected behavior

changedProperties in updated should contain modelValue only when array contents differ

Actual Behavior

It will probably fail on case 1 below.

Make a test that reflects #1026 (comment)

Assume we have a group with checkedIndex [0,1,4]
So spy on changedProperties and see if modelValue detects a change when:

  1. we do this.checkedIndex = [0,1,4] -> array ref changed, although we DO NOT expect it to be picked up in updated
  2. we do this.checkedIndex.push(2) -> array ref didn't change, but we DO expect it to be picked up in updated
@tlouisse
Copy link
Member Author

Problem: checking a single-choice child always triggers 2 events:

  • the newly checked option event
  • the unchecked event of the previously checked option
    Fixing this requires to evaluate sending the model-value-changed event asynchronously, and then we can compute our values based on all children events (the test above would run).
    But... by doing this (evaluating async) related select-rich / listbox tests that depend on synchronicity fail. It might be breaking in other contexts as well.

So it’s better to schedule till a refactor for v1: make parent group responsible for all bookkeeping, without waiting for children
events. This will be better for readibility, maintainability and performance.

See this example for how this could work: https://github.com/ing-bank/lion/blob/feat/manyMenus/packages/menu/src/InteractiveListMixin.js#L574

Test we could use in the future:

 it('only fires model-value-changed when selected indexes differ', async () => {
  const el = await fixture(html`
  <${tag} multiple-choice>
    <${optionTag} .choiceValue=${'10'} checked>Item 1</${optionTag}>
    <${optionTag} .choiceValue=${'20'}>Item 2</${optionTag}>
    <${optionTag} .choiceValue=${'30'}>Item 3</${optionTag}>
  </${tag}>
`);
  const spy = sinon.spy(() => {});
  el.addEventListener('model-value-changed', spy);
  expect(el.checkedIndex).to.eql([0]);
  el.checkedIndex = [0];
  await el.updateComplete;
  expect(spy).to.not.have.been.called;
  el.checkedIndex = [1];
  await el.updateComplete;
  expect(spy).to.have.been.calledOnce;
});

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

No branches or pull requests

1 participant