📝 Add ButtonGroup to Storybook

[martalalik]

Resolves #2419

[martalalik]

I would still consider to create a separate story for ButtonGroup 🤔

[martalalik]

Can someone review Split Button Group? I need some feedback, thanks.

[martalalik]

To fix position of menu button added:
style={{ position: 'absolute', height: '36px' }}

[martalalik]

Should Group include:

• role="group"
• aria-label or aria-labelledby

Looking into:

• role="group" description this should be mandatory for screenreader.
• aria-label and arial-labelledby description group should be given an explicit label. Make this as a a11y separate story 🤔

[oddvernes]

To fix position of menu button added: style={{ position: 'absolute', height: '36px' }}

Instead add display: flex to group and use same default button type for both buttons, but in the story, overwrite padding on the dropdown button to make it narrower and overwrite gap to 1px on group

[martalalik]

Do we want SplitButton in Compact story?

[oddvernes]

Do we want SplitButton in Compact story?

Make sure it works at least, but i don't think there will be an issue we only override left/right padding to something smaller so it'll just get less tall

[oddvernes]

• ToggleButton

I would rename Group to ButtonGroup. And that is all for this pr. ToggleButton is the upcoming component we havent made yet which is going to be something along the lines of ToggleButton from MUI / SegmentedControl from mantine / SelectorGroup from circuit

[mimarz]

Should we maybe expose Group via a proxy component as Button.Group to keep inline with the style of our other components?

Do we have other usages for this kind of grouping? Maybe Chip or Avatar?

[oddvernes]

Should we maybe expose Group via a proxy component as Button.Group to keep inline with the style of our other components?

Do we have other usages for this kind of grouping? Maybe Chip or Avatar?

But our pattern is Component.SubComponent whereas here it would be Child.Parent ? But I see what you mean, certain components could be wrapped in Component.Group if there is more

[mimarz]

Should we maybe expose Group via a proxy component as Button.Group to keep inline with the style of our other components?
Do we have other usages for this kind of grouping? Maybe Chip or Avatar?

But our pattern is Component.SubComponent whereas here it would be Child.Parent ? But I see what you mean, certain components could be wrapped in Component.Group if there is more

Yeah, naming is tricky 🙈 We could also name it Buttons or just leave it as Group and have a prop for toggling of rounding borders and gap instead. Many ways to do this.

I saw Mantine are using a combination for Component.SubComponent and Component.Parent, so they are using more of a way to show that some things are related to eachother 🤔

[martalalik]

I was to quick 🙈

[oddvernes]

If we one day decide to (or are asked) to make layout helper components like what mantine have, we should reserve very general names like "Group" for that i feel. But currently this is out of scope for eds as i understand (its too trivial and projects can quickly make them themselves anyway, it would make us responsible for everyone getting their layout correctly etc). ButtonGroup meanwhile only does one thing: its a wrapper that modifies the styling of buttons, nothing more, nothing less 🤷‍♂️

[oddvernes]

LGTM!

[mimarz]

After discussing on a call 24 august 2020, we decided the following:

• Group is renamed to ButtonGroup
• ButtonGroup is moved in under Button and exposed as a compound component (Button.Group)
• ButtonGroup stories are moved to Button.docs.mdx with the following stories
  • Split button
  • Orientation
  • Existing compact story is updated with ButtonGroup example

[martalalik]

Can anyone review it with this upcoming change?

[oddvernes]

missing a in aria

[martalalik]

done

[oddvernes]

Ok you misunderstood slightly; what i mean is above here is this (ria label): <Button.Group ria-label="button group compact">
I don't think you need aria label on the buttons

[martalalik]

Oh i see a typo!
Every button with icon should have an aria-label. Here

[oddvernes]

Oh true 😄

[oddvernes]

LGTM