fix ButtonGroup overflowButton logic

[mayank99]

Changes

Our current ButtonGroup overflow logic looks a bit flawed because it doesn't fully take into account the overflowButton itself. So I've adjusted the calculations to assume that overflowButton is one of the visible items.

Additionally, I've adjusted the values so that overflowPlacement=start will remove items from the beginning.

Expected values when overflowPlacement=start:

length	visibleCount	overflowStart
10	10	-
10	9 (including overflowButton)	1 (2 menu items)
10	8	2 (3 menu items)
10	7	3
10	6	4
10	5	5
10	4	6
10	3	7
10	2	8
10	1 (only overflowButton)	9 (10 items)

Expected values when overflowPlacement=end:

length	visibleCount	overflowStart
10	10	-
10	9 (including overflowButton)	8 (2 menu items)
10	8	7 (3 menu items)
10	7	6
10	6	5
10	5	4
10	4	3
10	3	2
10	2	1
10	1 (only overflowButton)	0 (10 items)

Testing

Unit tests pending.

Tested in playground using this code:

App.tsx

import * as React from 'react';
import { SvgPlaceholder, SvgMore } from '@itwin/itwinui-icons-react';
import {
  ButtonGroup,
  DropdownMenu,
  IconButton,
  MenuItem,
} from '@itwin/itwinui-react';

export default () => {
  const [overflowPlacement, setOverflowPlacement] = React.useState('end');
  const items = Array.from({ length: 10 }, (_, index) => (
    <IconButton key={index} label={`Item #${index}`}>
      <SvgPlaceholder />
    </IconButton>
  ));

  return (
    <>
      <fieldset>
        <legend>overflowPlacement</legend>
        <label>
          <input
            type='radio'
            name='placement'
            value='start'
            checked={overflowPlacement === 'start'}
            onChange={(e) => setOverflowPlacement(e.currentTarget.value)}
          />{' '}
          start
        </label>{' '}
        <label>
          <input
            type='radio'
            name='placement'
            value='end'
            checked={overflowPlacement === 'end'}
            onChange={(e) => setOverflowPlacement(e.currentTarget.value)}
          />{' '}
          end
        </label>
      </fieldset>

      <div
        style={{
          border: '1px solid',
          padding: 8,
          overflow: 'hidden',
          resize: 'horizontal',
        }}
      >
        <ButtonGroup
          orientation='horizontal'
          overflowPlacement={overflowPlacement as 'start' | 'end'}
          overflowButton={(overflowStart) => {
            return (
              <DropdownMenu
                menuItems={(close) => {
                  const length =
                    overflowPlacement === 'start'
                      ? overflowStart + 1
                      : items.length - overflowStart;

                  return Array.from({ length }, (_, _index) => {
                    const index =
                      overflowPlacement === 'start'
                        ? _index
                        : overflowStart + _index;

                    return (
                      <MenuItem
                        key={index}
                        onClick={close}
                        icon={<SvgPlaceholder />}
                      >
                        Item #{index}
                      </MenuItem>
                    );
                  });
                }}
              >
                <IconButton label='More'>
                  <SvgMore />
                </IconButton>
              </DropdownMenu>
            );
          }}
        >
          {items}
        </ButtonGroup>
      </div>
    </>
  );
};

Screen.Recording.2023-05-12.at.2.46.17.PM.mov

Docs

Briefly mentioned in migration guide: https://github.com/iTwin/iTwinUI/wiki/iTwinUI-react-v3-migration-guide#buttongroup

[gretanausedaite]

Could you write which v3 task you were working on when noticed this issue?
Also, could you mention related v3 task in all your PRs. It's hard to guess which PR goes with which task.

[mayank99]

@gretanausedaite Did you see the linked issue? I was working on this PR for v2 but it would have been a breaking change so moved to major release.

[gretanausedaite]

Did you see the linked issue?

Linked issue was missing v3 milestone so it wasn't in our v3 backlog. Sorry about that.

[LostABike]

I saw the original issue and tested this change, it is working as expected and should address the linked issue.

[mayank99]

I'll work on adding unit tests. I was looking for feedback on the calculations. Do they make sense? I was a bit unsure about the values in overflowPlacement=start case.

[LostABike]

I'll work on adding unit tests. I was looking for feedback on the calculations. Do they make sense? I was a bit unsure about the values in overflowPlacement=start case.

Yea I think it makes sense that when overflowPlacement=start, overflowButton will contain all the items that are not visible and show the rest (I'm going off the logic on line 115 and 125). Unit test would be great to sanity check the logic too.

[gretanausedaite]

I think calculations are a bit wrong. Storybook with overflowPlacement start

[mayank99]

I think calculations are a bit wrong. Storybook with overflowPlacement start

The values exposed by overflowButton callback should be correct, but the code for using that value will differ between start and end placements. I can update the story to handle both cases, but in the meantime check out the code snippet in PR description.

[gretanausedaite]

check out the code snippet in PR description.

That works perfectly!
It could be awesome if we showcased our users how to achieve certain functionality in docs site or storybook.

[gretanausedaite]

I believe unit tests are still pending.

[mayank99]

Updated unit tests and story.