Add Bootstrap v5 support for ButtonGroup

[simonihmig]

Checkbox and radio button groups are actually not buttons (referring to markup) anymore, but inputs (with .btn on the label): https://getbootstrap.com/docs/5.0/components/button-group/#checkbox-and-radio-button-groups. When they are neither of those, i.e. just grouped buttons, then they remain real <button>, even in BS5.

So this required some bigger changes. As our API stills exposes the "buttons" yielded from <BsButtonGroup> as instances of <BsButton>, we need to keep that API in place, i.e. support its arguments like @type and @size (which are fully supported on toggle buttons in BS5, even when they are technically no buttons anymore) but also our more exotic ones like @defaultText. So the yielded buttons still are subclasses of BsButton, however when they are radios/checkboxes in BS5, then the markup is just different (inputs vs. buttons).

I briefly considered making <BsButtonGroup::Button> not extend BsButton, but just a light wrapper that uses it, but that would make supporting all the button APIs for these input-based buttons way harder, would basically have to duplicate all the button API for those. So I think this is a reasonable solution, even if it might look a bit confusing at first (for us maintainers, not users).

[jelhan]

I briefly considered making <BsButtonGroup::Button> not extend BsButton, but just a light wrapper that uses it, but that would make supporting all the button APIs for these input-based buttons way harder, would basically have to duplicate all the button API for those.

What do you mean by duplicate the button APIs? Would it be more than passing all arguments through? Something like this:

// addon/components/bs-button-group/button.hbs

<BsButton
  @size={{@size}}
  @type={{@type}}
  @defaultText={{@defaultText}}
  @fulfilledText={{@fulfilledText}}
  @rejectedText={{@rejectedText}}
  @pendingText={{@pendingText}}
  as |button|
>
  {{yield button}}
</BsButton>

[simonihmig]

What do you mean by duplicate the button APIs? Would it be more than passing all arguments through? Something like this:

Yes, that's what I did for the preparatory work to not extend BsButton first. But for BS5 toggle buttons, you cannot reuse <BsButton>, as that renders a button, but we need <input> and <label>. So if there was a separate component for this, which does not extend from BsButton, it would have to reimplement all these things as @defaultText. Or do you see some other way?

[jelhan]

What do you mean by duplicate the button APIs? Would it be more than passing all arguments through? Something like this:

Yes, that's what I did for the preparatory work to not extend BsButton first. But for BS5 toggle buttons, you cannot reuse <BsButton>, as that renders a button, but we need <input> and <label>. So if there was a separate component for this, which does not extend from BsButton, it would have to reimplement all these things as @defaultText. Or do you see some other way?

Now I got it. If <BsButtonGroup::Button> would not extend <BsButton> it would either a) need to duplicate code to support all its features support or b) <BsButton> would need to support rendering as <input> and <label>. Both doesn't seem to be acceptable trade-offs. Agree that extending from <BsButton> is the best choice we have currently.

We could maybe revisit this one in case we deprecate and remove some of the arguments. E.g. I'm not sure if we need @defaultText, @fulfilledText, @rejectedText and @pendingText anymore. Consumer could achieve the same using the yielded state. But that's a separate topic.

[jelhan]

Looks good to me. But I haven't reviewed in depth to be honest.

Left a general question if we should split tests between bootstrap versions or better have different code paths within the same test if required. I'm tending towards the second. But not a blocker for this specific pull request in my opinion.

[jelhan]

Did you considered not splitting into two separate tests but only adjust this assertion accordingly to Bootstrap version? Something like:

        assert.equal(
          isBootstrap(5)
            ? this.element.querySelector(`input[type="checkbox"]:nth-of-type(${k + 1})`).checked
             : this.element.querySelector(`button:nth-child(${k + 1})`).classList.contains('active'),
          0 === k,
          'only button with same value is active'
        );

Similar for the other tests.

I would tend towards using the same test with version dependent code paths unless a feature is only supported for some Bootstrap versions. I fear maintaining the tests gets otherwise more difficult. Especially it would be too easy to miss test coverage for one Bootstrap version.

[simonihmig]

Well, yeah, I was more concerned that tests start to get unreadable, so was more leaning into the other direction. Like when the only difference is a class name, using a helper function as we do seems good, but here we have to write even different code (. checked vs. .classList.contains()).

But there is no clear winner here I guess, more preferences...