feat: add checkbox indeterminate

[MatthieuLebigre]

What I did

Based on #1128, and inspired by #1134, I open this PR to add the checkbox with indeterminate state.

What is different from #1134 is mostly that I added the nested checkbox in the slot of the lion-checkbox-indeterminate instead of being siblings.

This my first PR in lion, so any feedback will be appreciated, I will update the PR based on your comments.

[changeset-bot]

🦋 Changeset detected

Latest commit: 8d2b251

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 21 packages

Name	Type
@lion/checkbox-group	Minor
@lion/form-core	Patch
@lion/listbox	Patch
@lion/switch	Patch
@lion/form-integrations	Patch
@lion/combobox	Patch
@lion/fieldset	Patch
@lion/input-amount	Patch
@lion/input-date	Patch
@lion/input-datepicker	Patch
@lion/input-email	Patch
@lion/input-iban	Patch
@lion/input-stepper	Patch
@lion/input	Patch
@lion/radio-group	Patch
@lion/select-rich	Patch
@lion/select	Patch
@lion/textarea	Patch
@lion/validate-messages	Patch
@lion/form	Patch
@lion/input-range	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[CLAassistant]

All committers have signed the CLA.

[jorenbroekema]

Hey, I think the behaviour looks good and I like the API way better than my POC, good idea to add a small hook afterTemplate and use it in CheckboxIndeterminate with <slot>!

Indeed what @gerjanvangeest said about sibling indeterminate checkboxes is true, we need to ensure that we only check subcheckboxes that are children of the indeterminate one.

So to continue on this PR:

• Subcheckboxes only consider formElements that are children, see my suggestion in the reply on that comment
• Tests (let us know if you get stuck! we can help):
  • Toggling indeterminate toggles/syncs all children
  • Indeterminate is true only if more than 0 and less than all child checkboxes are checked.
  • The indeterminate is checked true if all children are checked true
  • The indeterminate is checked false (and indeterminate also false) if all children are checked false
  • Some edge case tests like testing sibling indeterminate checkbox groups, nested indeterminate checkbox groups, and put extra <div> in between to see if it still works with arbitrary html in between
• Fix type errors, but we can help you with that if you're not familiar with JSDocs / TSC combination 👍
• Make __toggleChecked and __parentFormGroup protected + release minor version with breaking changelog

Later, in a separate PR:

• indeterminate checkbox toggling between tri-state and saving the subcheckboxes state, issue: [checkbox-group] indeterminate checkbox toggling between tri-state and saving the subcheckboxes state #1189

---

One last thing, do we want to support:

<lion-checkbox-group name="scientists[]" label="Favorite scientists">
  <lion-checkbox-indeterminate label="Scientists">
    <lion-checkbox label="Isaac Newton" .choiceValue=${'Isaac Newton'}></lion-checkbox>
    <lion-checkbox label="Galileo Galilei" .choiceValue=${'Galileo Galilei'}></lion-checkbox>
    <lion-checkbox-indeterminate label="Old Greek scientists">
      <lion-checkbox label="Archimedes" .choiceValue=${'Archimedes'}></lion-checkbox>
      <lion-checkbox label="Plato" .choiceValue=${'Plato'}></lion-checkbox>
      <lion-checkbox label="Pythagoras" .choiceValue=${'Pythagoras'}></lion-checkbox>
    </lion-checkbox-indeterminate>
  </lion-checkbox-indeterminate>
</lion-checkbox-group>

That would mean that selecting the very last checkbox (Pythagoras) toggles indeterminate of Old Greek scientists. But the question is than if the parent indeterminate checkbox of Scientists now becomes indeterminate or not? I think yes, probably.

With my suggestion in the comment earlier to do this.contains(checkbox) check for subcheckboxes, I think this will work instantly. Checking a far nested child will trigger all parent indeterminates, as intended. I think 🙈

So if we support that use case, we also will need some tests for that of course :)

[jorenbroekema]

I couldn't help myself, made some improvements MatthieuLebigre#1 feel free to check them out before merging to this. I just really wanted to see if the nested indeterminates would work and they do 🎉 😱

[MatthieuLebigre]

Hello all !! Thanks all for the comments and suggestions ? I'll fix everything when I'll come back from vacation (beginning of January) :)

[jorenbroekema]

Huh thanks to Thijs' comment I noticed we dont have to listen to input change event manually, we can reuse the __toggleChecked method:

  /**
   * @override ChoiceInputMixin
   * first set subCheckboxes if event came from itself
   * then set own checked + indeterminate state
   * @param {Event} ev
   */
  __toggleChecked(ev) {
    if (this.disabled) {
      return;
    }

    if (ev.target === this) {
      this._subCheckboxes.forEach(checkbox => {
        // eslint-disable-next-line no-param-reassign
        checkbox.checked = this._inputNode.checked;
      });
    }
    this._setOwnCheckedState();
  }

I guess __parentFormGroup and __toggleChecked should be made protected with a single underscore 🙈 can fix that too while we're at it

I updated my PR to your fork @MatthieuLebigre as well as the checklist I wrote above 👍 have a nice holiday and see you in 2021!

[tlouisse]

Huh thanks to Thijs' comment I noticed we dont have to listen to input change event manually, we can reuse the __toggleChecked method:

  /**
   * @override ChoiceInputMixin
   * first set subCheckboxes if event came from itself
   * then set own checked + indeterminate state
   * @param {Event} ev
   */
  __toggleChecked(ev) {
    if (this.disabled) {
      return;
    }

    if (ev.target === this) {
      this._subCheckboxes.forEach(checkbox => {
        // eslint-disable-next-line no-param-reassign
        checkbox.checked = this._inputNode.checked;
      });
    }
    this._setOwnCheckedState();
  }

I guess __parentFormGroup and __toggleChecked should be made protected with a single underscore 🙈 can fix that too while we're at it

I updated my PR to your fork @MatthieuLebigre as well as the checklist I wrote above 👍 have a nice holiday and see you in 2021!

Think I created something similar yesterday: https://github.com/ing-bank/lion/blob/feat/checkbox-indeterminate-enhancements/packages/checkbox-group/src/LionCheckboxIndeterminate.js#L85

This handles readonly and tristate (it captures indeterminate states, also taking into account users 'meddling' with the captured state by (un)selecting all).

Maybe will have a deeper look on Monday. If you can't wait on that, that's fine with me :) we can also add those details later

[MatthieuLebigre]

@jorenbroekema I merged your PR, it should be easier to track all changes in this one.

[MatthieuLebigre]

I did a few changes/fix based on the feedback. I also started to add unit tests. I'll continue on that topic, but I believe for the code itself it should be fine now

[jorenbroekema]

Tests are looking good @MatthieuLebigre ! Just a few left, I updated the checklist. After those last tests are in, I'll do a full review and finish things up and release 👍

[MatthieuLebigre]

Tests are looking good @MatthieuLebigre ! Just a few left, I updated the checklist. After those last tests are in, I'll do a full review and finish things up and release 👍

Done @jorenbroekema

[jorenbroekema]

Yay, thanks @MatthieuLebigre for contributing and for collaborating on this with us 🤗