[DTO-5100] BpkChipGroup #3198

Iain530 · 2024-01-25T16:32:24Z

The BpkChipGroup is used to group a list of chips (bpk-component-chip) in either a wrapped layout or rail (single row with scroll. See image above or Figma or all examples.

Changes

Created new component BpkChipGroup

Remember to include the following changes:

Ensure the PR title includes the name of the component you are changing so it's clear in the release notes for consumers of the changes in the version e.g [KOA-123][BpkButton] Updating the colour
README.md (If you have created a new component)
Component README.md
Tests
Storybook examples created/updated
For breaking changes or deprecating components/properties, migration guides added to the description of the PR. If the guide has large changes, consider creating a new Markdown page inside the component's docs folder and link it here

github-actions · 2024-01-25T16:36:38Z

	Warnings
⚠️	Package source files (e.g. `packages/package-name/src/Component.js`) were updated, but snapshots weren't. Have you checked that the tests still pass?

Browser support

If this is a visual change, make sure you've tested it in multiple browsers.

Generated by 🚫 dangerJS against 822bdab

Iain530 · 2024-01-25T16:50:44Z

packages/bpk-component-chip-group/src/BpkChipGroup.tsx

+export type CommonProps = {
+  ariaLabel?: string;
+  ariaLabelledBy?: string;
+  type: ChipGroupType;
+  className?: string | null;
+  style?: ChipStyleType;
+};
+
+export type ChipGroupProps = {
+  chips: ChipItem[];
+  stickyChip?: ChipItem;
+  _isSingleSelect?: boolean; // TODO: is there a better way for internal only props?
+} & CommonProps;


These props differ a bit from the API in the figma: https://www.figma.com/file/irZ3YBx8vOm16ICkAr7mB3/Backpack?node-id=30119%3A42147&mode=dev

ariaLabel, ariaLabelledBy: we require exactly one of these to be passed when using role="listbox". I'd like to explore a better way than having both of these as optional props and emitting a warn when neither or both are passed.

_isSingleSelect: This is just used for the SingleSelectChipGroup to control the aria-multiselectable attribute which is also required for listbox. This should never be used by a consumer of the component but it feels like there should also be a better way to do this. Any suggestions would be welcome.

This component is very similar to https://react-spectrum.adobe.com/react-spectrum/ActionGroup.html drawing inspiration from them they have a selectionMode prop. That feels like what _isSingleSelect is describing, however, gives a more semantic meaning.

I've renamed this to ariaMultiselectable for now as this id all the prop controls, the actual selection mode depends on which component is used.

Iain530 · 2024-01-25T17:00:34Z

packages/bpk-component-chip-group/src/BpkChipGroup.tsx

+export type SingleSelectChipItem = {
+  text: string;
+  accessibilityLabel?: string;
+  leadingAccessoryView?: ReactNode;
+  className?: string;
+  [rest: string]: any; // Inexact rest. See decisions/inexact-rest.md
+};
+
+export type ChipItem = {
+  component?: (props: BpkSelectableChipProps) => ReactElement;
+  onClick?: (selected: boolean, index: number) => void;
+  selected?: boolean;
+} & SingleSelectChipItem;


The shape of this is a bit different to the figma at the moment but I've tried to keep it as close as possible: https://www.figma.com/file/irZ3YBx8vOm16ICkAr7mB3/Backpack?node-id=30119%3A42328&mode=dev

Currently differs by:

icon replaced by leadingAccessoryView as this is the prop in BpkSelectableChipProps, although this could be renamed to icon for this prop if preferred.

component instead of type but could be made to use an enum to choose from selectable, dropdown and dismissable chips.

Since these just passed through to a BpkChip component then a ...rest param seemed sensible at the time, although may not be neccessary now I'm looking back.

packages/bpk-component-chip-group/src/BpkChipGroup.tsx

Iain530 · 2024-01-25T17:16:02Z

packages/bpk-component-chip-group/src/BpkChipGroup.tsx

+  );
+};
+
+export const BpkChipGroupState = ({ chips, ...rest }: ChipGroupProps) => {


The figma didn't mention that we'd expose stateful versions of these components but I needed them for testing anyway so they're here. If we'd prefer not to expose these then they can easily be moved to the example stories.

packages/bpk-component-chip-group/src/Nudger.tsx

packages/bpk-component-chip-group/src/BpkChipGroup.module.scss

mungodewar · 2024-01-26T14:31:25Z

Following on the conversation from slack: https://skyscanner.slack.com/archives/C0JHPDSSU/p1706274673682399?thread_ts=1706259605.819169&cid=C0JHPDSSU

I’m thinking that the composable pattern is better, as the prop is a hard coupling, however, we could make it looser by the ChipGroup rendering children and managing Chip props via something like context.

eg there would be no need for the custom stickyChip code, the composition would be enough I think?

<BpkChipGroup>
    <BpkChip sticky />
    {myOtherChips.map(chip => <BpkChip {...chip} />)}
</BpkChipGroup>

The dismissed example becomes a lot more concise:

  return (
    <BpkChipGroup>
        <BpkChip disabled />
        <BpkChip dismissable />
        <BpkDropdownChip >
        <BpkChip />
        <BpkChip selected/>
    </BpkChipGroup>
  );

Semantically, the BpkChipGroup manages a group of BpkChips. This reflects other web apis like <select> and <option>.

I can think of future requirements/designs that "clumps" chips together or seperate them. Having this api allows easy extensibility & no change to BpkChipGroup would be required:

  return (
    <BpkChipGroup>
        <BpkChip disabled />
        <BpkChip dismissable />
        <BpkChipSpacer />
        <BpkDropdownChip >
        <BpkChip />
        <BpkChip selected/>
    </BpkChipGroup>
  );

or

  return (
    <BpkChipGroup>
        <BpkChipSpacer >
             <BpkChip disabled />
             <BpkChip dismissable />
        </BpkChipSpacer >
        <BpkDropdownChip >
        <BpkChip />
        <BpkChip selected/>
    </BpkChipGroup>
  );

mungodewar · 2024-01-26T14:32:22Z

There are a few different types of BpkChipGroup, could you give me a TLDR on when a consumer would use which one and why, are some of these just internal maybe?

mungodewar · 2024-01-26T14:33:26Z

I'm interested in onItemClick prop, is there a reason that we don't want that on all the different version of BpkChipGroup?

mungodewar · 2024-01-26T15:32:04Z

Finding: https://react-spectrum.adobe.com/react-spectrum/ActionGroup.html & how similar it is to this component, it might be worth taking a look and doing a contrast and compare to their solution - particularly around the whole api approach and how they manage it.

Iain530 · 2024-02-02T15:44:46Z

Following on the conversation from slack: skyscanner.slack.com/archives/C0JHPDSSU/p1706274673682399?thread_ts=1706259605.819169&cid=C0JHPDSSU

I’m thinking that the composable pattern is better, as the prop is a hard coupling, however, we could make it looser by the ChipGroup rendering children and managing Chip props via something like context.

eg there would be no need for the custom stickyChip code, the composition would be enough I think?

This conversation continued on slack a bit, the main reason for having the ChipItems is to give us more control over the BpkChip components which I think we lose if they become composable?

See:

I understand this would mean it's harder to migrate to using this component though so if there's a potential middle ground where we still can have control over the props we need but with this pattern that might be ideal. Although I don't know if that's possible?

Iain530 · 2024-02-02T15:51:45Z

There are a few different types of BpkChipGroup, could you give me a TLDR on when a consumer would use which one and why, are some of these just internal maybe?

Yes, so there are two main types MultiSelect and SingleSelect:

MultiSelect means that any number of chips can be selected
SingleSelect means that only a single chip can be selected (controlled through the selectedIndex prop)

These two components are not stateful so a consumer should use these if they want to manage the chip states themselves.

Then there are also stateful versions of both of these which wrap the stateless ones and make them stateful, simplifying the job of the consumer if their use case is simple. The stateful ones weren't included in the design but I needed to write them for testing anyway so it seemed like a good idea to include them. If we'd rather not though I can take them out or refactor them to use a wrapping pattern like this:

backpack/packages/bpk-component-info-banner/src/withBannerAlertState.d.ts

Line 25 in 773b1e5

declare const withBannerAlertState: <

The storybook contains examples of both single and multi select (stateful).

* Split definitions for group types * Split sub-component definitions to reduce conditional logic * PR comment action and TS fix --------- Co-authored-by: Iain Cattermole <22656572+Iain530@users.noreply.github.com>

* Remove label padding * Make nudger labels required

github-actions · 2024-05-02T13:02:02Z

Visit https://backpack.github.io/storybook-prs/3198 to see this build running in a browser.

anambl · 2024-05-06T14:23:41Z

Hey guys what's the status of this? What needs to happen to unblock this? 😄 @Iain530 @olliecurtis @mungodewar

olliecurtis · 2024-05-06T14:34:51Z

Hey guys what's the status of this? What needs to happen to unblock this? 😄 @Iain530 @olliecurtis @mungodewar

I need to come and review now that this has been updated since my last review :)

Iain530 · 2024-05-07T09:58:25Z

Hey guys what's the status of this? What needs to happen to unblock this? 😄 @Iain530 @olliecurtis @mungodewar

I need to come and review now that this has been updated since my last review :)

Thanks @olliecurtis! I think we should have addressed everything except the documentation questions around storybook/props comments.

Iain Cattermole added 6 commits September 21, 2023 16:02

wip

74516e6

Merge branch 'main' of github.com:Skyscanner/backpack into DTO-4223

47c712a

Create nudger for desktop

17d03c7

Refactor chip groups to better match api

92ec095

More examples, support for dismissable chips, tests

40480ef

More WIP, Update docs, add tests, add a11y roles, add TODOs

50ffcb4

Iain530 added the minor Non breaking change label Jan 25, 2024

Merge branch 'main' into DTO-4223

b5690b3