[BDES-96] [BpkCardList] Add Grid and Stack Layouts #3697
Conversation
|
Visit https://backpack.github.io/storybook-prs/3697 to see this build running in a browser. |
FireRedNinja
changed the title
Browser supportIf this is a visual change, make sure you've tested it in multiple browsers. |
|
Visit https://backpack.github.io/storybook-prs/3697 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3697 to see this build running in a browser. |
| buttonHref="" | ||
| onButtonClick={() => null} | ||
| cardList={cards(DestinationCard)} | ||
| layoutDesktop="grid" |
There was a problem hiding this comment.
I'd create some constants and export them for all these to avoid consumers having to hardcode these
There was a problem hiding this comment.
small typo in the file name?
| onButtonClick, | ||
| title, | ||
| } = props; | ||
|
|
||
| const button = buttonText && ( | ||
| <BpkButtonV2 onClick={onButtonClick} href={buttonHref}>{buttonText}</BpkButtonV2> | ||
| const button = buttonText && accessory !== 'button' && ( |
There was a problem hiding this comment.
likewise with this - a constant would be useful to avoid consumers misusing the component due to a typo
| } = props; | ||
|
|
||
| let defaultInitiallyShownCards: number; | ||
| if (accessory === ACCESSORY_TYPES.Expand) { |
There was a problem hiding this comment.
I don't think this depends on the accessory. From the API table:
3 cards displayed by default, but you can change it.
There was a problem hiding this comment.
This is for the case when more than 3 cards are given but the user don't want to use an expand accessory. It would look confusing for the user as it would simply look like the component is broken with only 3 cards showing
| collapsed={collapsed} | ||
| setCollapsed={setCollapsed} | ||
| > | ||
| {expandText || ''} |
There was a problem hiding this comment.
I wonder if there's a way in TS to ensure expandText is passed when accessory is expand 🤔 likewise for button and button text
| const buttonOnClick = () => { | ||
| if (collapsed) { | ||
| showContent(); | ||
| setCollapsed(false); |
There was a problem hiding this comment.
I think it's better not to directly change the state here and instead pass a callback from the parent for the onClick functionality. In fact, I think showContent handles that already
Remember to include the following changes:
[KOA-123][BpkButton] Updating the colourREADME.md(If you have created a new component)README.mdScreenshots
Grid with Header Button
Stack with Header Button
Grid with Button Accessory
Stack with Button Accessory
Grid with Expand Accessory
Stack with Expand Accessory