Conversation
🦋 Changeset detectedLatest commit: 06edb74 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
the
integration-tests: recommended
|
liuliu-dev
added
the
update snapshots
github-actions
bot
removed
the
update snapshots
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
lukasoppermann
left a comment
lukasoppermann
left a comment
There was a problem hiding this comment.
@liuliu-dev design wise:
- heading, please use
--text-title-shorthand-small - for body copy:
--text-body-shorthand-medium - for the gap please use a variable from the stack: https://primer.style/product/primitives/size/#stack
I think we can remove the fallback values, they are not needed and don't exist anymore.
Agent-Logs-Url: https://github.com/primer/react/sessions/68b1fedd-5be4-41d5-be1b-87618778b53c Co-authored-by: lukasoppermann <813754+lukasoppermann@users.noreply.github.com>
Done in 375b519.
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@liuliu-dev should we tie the padding to the content-top, instead of icon bottom? 
|
|
Uh oh! @lukasoppermann, at least one image you shared is missing helpful alt text. Check #7723 (comment) to fix the following violations:
Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
|
|
This is also a use case: https://primer-1b76de82d9-13348165.drafts.github.io/storybook/?path=/story/components-card--custom-content 
I am wondering if we should expect users to "bring their own padding" or if we should have a padding inside the card by default and a prop or a way to easily remove the default padding? CC: @siddharthkp ? |
|
Uh oh! @lukasoppermann, at least one image you shared is missing helpful alt text. Check #7723 (comment) to fix the following violations:
Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
|
Amazing! Thank you so much! |
|
Thanks @lukasoppermann! Here's what I've updated based on your feedback:
|
|
@liuliu-dev perfect, thank you. |
I think bring your padding is a pattern we don't follow anywhere right now because it can lead to variation that is visibly inconsistent. Something we can take from PageLayout is the idea of |
The prop is now implemented, this makes sense to me. @siddharthkp the Pagelayout does not make total sense to me, because there seems to be no
This is important! 👍 |
My bad, the values in the docs are
I've come into this blind, so I don't actually know what's going on or what we are trying to do 😅 I would say however, we should put this in experimental and not root index right away |
packages/react/src/index.ts
Outdated
| CardDescriptionProps, | ||
| CardMenuProps, | ||
| CardMetadataProps, | ||
| } from './Card' |
There was a problem hiding this comment.
Should we export this from src/experimental/index until we build some confidence in the API with some usage?
There was a problem hiding this comment.
Whatever you folks feel is best. Do we have a clear path for how we move this out of experimental? It feels like stuff stays in experimental for way to long some times.
There was a problem hiding this comment.
Makes sense to keep it in experimental for now. It’s mostly upstreamed from Pacer so it’s not totally new, but the padding prop is new, if it looks good in usage we can move it out pretty quickly.
|
Figma component: https://github.com/github/primer/issues/6537 |
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/18126 |
Upstream the Card component from
Pacerinto@primer/react.Issue: https://github.com/github/primer/issues/6467
Changelog
New
Card— A styled container component with border, box-shadow, and border-radius; also supports custom content without subcomponents (plain container mode)Card.Icon— Renders a decorative icon in the card header (defaults toaria-hidden, supportsaria-labelfor meaningful icons)Card.Image— Renders an edge-to-edge image in the card headerCard.Heading— Renders a heading (defaults to h3, configurable viaasprop)Card.Description— Renders a description below the headingCard.Menu— Slot for overlay menu (positioned absolute top-right)Card.Metadata— Renders metadata content below the descriptionChanged
PlaygroundStorybook story updated with boolean toggle controls forCard.IconandCard.MetadataCustomContentStorybook story added to demonstrate usingCardas a plain styled container with arbitrary children (no subcomponents)Rollout strategy
Testing & Reviewing
Merge checklist