Split definitions for group types #3370
Conversation
| @@ -70,131 +70,172 @@ export type ChipItem = { | |||
| hidden?: boolean; | |||
| } & SingleSelectChipItem; | |||
|
|
|||
| export type InternalProps = { | |||
There was a problem hiding this comment.
Props common to both components which are not exposed to external consumers
| export type CommonProps = { | ||
| label?: string; | ||
| ariaLabel?: string; | ||
| type?: ChipGroupType; |
There was a problem hiding this comment.
This is now only used where we define the union of component types
| }; | ||
|
|
||
| export type ChipGroupProps = { | ||
| chips: ChipItem[]; | ||
| export type RailChipGroupProps = { | ||
| stickyChip?: ChipItem; |
There was a problem hiding this comment.
Define props that are specific to rail version
| trailingNudgerLabel?: string; | ||
| } & CommonProps; | ||
|
|
||
| export type WrapChipGroupProps = { |
There was a problem hiding this comment.
There isn't anything specific in here but open question whether we like having a marker type to describe the scenario
| } & CommonProps; | ||
|
|
||
| export type ChipGroupProps = (RailChipGroupProps & { type: typeof CHIP_GROUP_TYPES.rail } | WrapChipGroupProps & { type: typeof CHIP_GROUP_TYPES.wrap }); |
There was a problem hiding this comment.
We could move the type field into the respective sub-types but it isn't used by any direct consumers so eslint isn't happy with that - defining it with the union here covers what we need without exposing it to places that don't use it
There was a problem hiding this comment.
Reading this I'm expecting typeof CHIP_GROUP_TYPES.rail to be string, instead of 'rail' which is what I think we want?
There was a problem hiding this comment.
No, thanks to the as const on the CHIP_GROUP_TYPES (line 39) the type is specifically rail instead of string
Here is a preview of the evaluated type:
|
|
||
| const BpkChipGroup = ({ | ||
| const Chip = ({ ariaMultiselectable, chipIndex, chipItem, chipStyle }: { chipIndex: number, chipItem: ChipItem, chipStyle: ChipStyleType, ariaMultiselectable: boolean }, index: number) => { |
There was a problem hiding this comment.
Extracted from the inline func to build a chip
There was a problem hiding this comment.
Can we remove the index param now we have chipIndex?
There was a problem hiding this comment.
Yes, well spotted 👍
| ); | ||
| } | ||
|
|
||
| const ChipGroupContent = ( |
There was a problem hiding this comment.
Extracted from the common core legend component of each variety of chipgroup
| </fieldset> | ||
| ); | ||
|
|
||
| const RailChipGroup = ({ |
There was a problem hiding this comment.
Less conditionals now we know this is a rail group
| ); | ||
| }; | ||
|
|
||
| const WrapChipGroup = ({ |
There was a problem hiding this comment.
Simpler logic now we know doesn't support rail logic
| chips={chips} | ||
| label={label} />; | ||
|
|
||
| const BpkChipGroup = ({ label, type, ...rest }: ChipGroupProps) => { |
There was a problem hiding this comment.
External interface which now defers to the respective sub-types
| } & CommonProps; | ||
|
|
||
| export type ChipGroupProps = (RailChipGroupProps & { type: typeof CHIP_GROUP_TYPES.rail } | WrapChipGroupProps & { type: typeof CHIP_GROUP_TYPES.wrap }); |
There was a problem hiding this comment.
Reading this I'm expecting typeof CHIP_GROUP_TYPES.rail to be string, instead of 'rail' which is what I think we want?
|
|
||
| const BpkChipGroup = ({ | ||
| const Chip = ({ ariaMultiselectable, chipIndex, chipItem, chipStyle }: { chipIndex: number, chipItem: ChipItem, chipStyle: ChipStyleType, ariaMultiselectable: boolean }, index: number) => { |
There was a problem hiding this comment.
Can we remove the index param now we have chipIndex?
|
|
||
| export type SingleSelectProps = { | ||
| chips: SingleSelectChipItem[]; | ||
| selectedIndex?: number; | ||
| onItemClick?: (item: SingleSelectChipItem, selected: boolean, index: number) => void, | ||
| } & CommonProps; | ||
|
|
||
| } & ChipGroupProps; |
There was a problem hiding this comment.
Want to use the entire ChipGroupProps because consumer can choose type and then may need to provided specific args based on selection
There was a problem hiding this comment.
I think this might have caused the chips prop to now include the ChipItem as well as SingleSelectChipItem
| leadingNudgerLabel?: string; | ||
| trailingNudgerLabel?: string; |
There was a problem hiding this comment.
These two labels should now be required since they were previously required for rail but not needed for wrap. Now this type is rail only they can be required and the default of "" removed
Splitting up definitions of the group types to reduce logic