-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[Card] Add support for multiple secondary actions in footer and the option to disable them #1625
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
794990d
to
1db44b1
Compare
1db44b1
to
d3f5ac0
Compare
src/components/Card/README.md
Outdated
```jsx | ||
<Card | ||
title="Shipment 1234" | ||
secondaryFooterAction={{content: 'Cancel shipment'}} |
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.
as the old API is deprecated, I figured I'd update the existing singular example
Not sure why the lines below are showing up as new though
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.
💯
d3f5ac0
to
6147e49
Compare
Hey @qq99 I have asked for translations. From a quick glance this looks okay. Have you tophatted this change in mobile, multiple browsers and for accessibility? |
6147e49
to
8ea3bfd
Compare
We've been chatting about using |
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 prop deprecation/replacement makes sense to me. Code and 🎩 👀 solid 👍 Just a couple typo fixes and a few content changes suggested.
I rebased your branch on master when I checked it out locally. The only real conflict was a content update from "Cancel shipment" to "Edit shipment" in one of README examples since there's now a specific example for a card with a destructive footer action. Didn't want to push it up in case you made any changes locally too, but happy to push it up if you run into issues.
expect(button.text()).toBe('a'); | ||
}); | ||
}); | ||
|
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.
Should there be a test for the disclosure text rendering a default value if secondaryFooterActionsDisclosureText
is unset?
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.
There is in a way, in popover works when disclosure button is clicked
:
const disclosureButton = card.find(Button).first();
expect(disclosureButton).toHaveLength(1);
expect(disclosureButton.text()).toBe('More');
afbc5b7
to
4ec3a56
Compare
4ec3a56
to
410ee75
Compare
I think I got all your changes @chloerice 🤞 |
410ee75
to
30063d0
Compare
Should merge after #1622 to minimize merge conflictsWHAT is this pull request doing?
secondaryFooterActions
, and adds deprecation notes tosecondaryFooterAction
(singular)primaryFooterAction
andsecondaryFooterAction
to be disableableWith an array of actions, you can now easily achieve this:

With an array of just 1 action, it falls back to rendering as if you had supplied a

secondaryFooterAction
(singular):Here's a picture of disabling the secondary, and of a custom disclosure text:

How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Copy-paste this code in
playground/Playground.tsx
:🎩 checklist
README.md
with documentation changes