-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[AlphaCard] Add storybook examples for Card compositions #9003
Conversation
size-limit report 📦
|
2232d48
to
2cfe9e9
Compare
2cfe9e9
to
5b00e7b
Compare
5b00e7b
to
4c45271
Compare
plainButton?: boolean; | ||
} | ||
|
||
export function secondaryActionsFrom({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was added because secondary actions rendered a lot of components, so it reduces the amount of copy/paste users have to do to recreate multiple secondary actions. The naming convention follows the way we have buttonFrom
and buttonsFrom
for Button utils for consistency.
I was considering adding another util for primary actions but the code for that is pretty straightforward and minimal. Open to feedback on the secondaryActionsFrom
util and whether I should supplement it with a primaryActionsFrom
util as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a tough one but I have a few points
- I wouldn't expect this helper to come from the card component
- Do we need a separate helper?
- Can we make this work instead:
{buttonsFrom([
{content: 'Gross Sales', plain: true},
{
content: 'Net Sales',
plain: true,
connectedDisclosure: {
accessibilityLabel: 'View Sales',
actions: [{content: 'View Sales'}],
},
},
])}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ComplexAction
type doesn't support a connected disclosure, so it'd be a larger refactor to support #3.
If it makes more sense, I can move the logic from the AlphaCard util into the Buttons util and refactor it so that it's not worded specifically for secondary actions but for any button that requires a disclosure to display more actions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The types don't support it and they styling is off but it does render and I think it's the right way forward. Moving this util to buttons with a more specific name would be fine in the interim 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try extending the types to support connected disclosures in the ComplexAction
and refactor some of the utils and Button so it's not left in an interim state 👍
Closing in favor of #11201. |
WHY are these changes introduced?
Part of #87817.
With
AlphaCard
replacingCard
, we should have storybook examples for the different ways it can be composed with the other layout components to recreate the UI that the deprecated Card component supported via props.WHAT is this pull request doing?
How to 🎩
I tophatted all the new stories against existing Card stories at all breakpoints.
For easier tophatting, I added the code I used to test below for the Playground, and those can be tested across all breakpoints.
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Copy-paste this code in
playground/Playground.tsx
:🎩 checklist
README.md
with documentation changes