Skip to content
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

Feat(web): Introduce Card component #DS-1397 #1751

Draft
wants to merge 5 commits into
base: integration/card
Choose a base branch
from

Conversation

adamkudrna
Copy link
Contributor

Description

Additional context

Issue reference

@adamkudrna adamkudrna self-assigned this Nov 12, 2024
@github-actions github-actions bot added the feature New feature or request label Nov 12, 2024
@adamkudrna adamkudrna force-pushed the feat/web-card branch 2 times, most recently from 4961e78 to 1e80a46 Compare November 12, 2024 15:09
Copy link

netlify bot commented Nov 12, 2024

Deploy Preview for spirit-design-system-storybook ready!

Name Link
🔨 Latest commit 028a33d
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system-storybook/deploys/673cf1c4bbaf8500085774fa
😎 Deploy Preview https://deploy-preview-1751--spirit-design-system-storybook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Nov 12, 2024

Deploy Preview for spirit-design-system ready!

Name Link
🔨 Latest commit 028a33d
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system/deploys/673cf1c47ebce9000839245c
😎 Deploy Preview https://deploy-preview-1751--spirit-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 96 (no change from production)
Accessibility: 91 (🔴 down 2 from production)
Best Practices: 100 (no change from production)
SEO: 91 (🟢 up 1 from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@literat
Copy link
Collaborator

literat commented Nov 12, 2024

❤️

packages/web/src/scss/components/Card/_CardActions.scss Outdated Show resolved Hide resolved
}

// 1.
.CardBody p {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need this only when the cardLink is present, right? And if the heading is not a cardlink (only the media for example), shouldn't the heading be selectable too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Yes, fixed.
  2. No, there is a link inside the title (or should be).

.CardLink:first-of-type {
@include links.stretch();

text-decoration: none;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this prop be applied to all cardlinks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. It's intentionally bound to the stretching feature so the underline doesn't flicker on hover. :first-of-type is here to make sure just the first .CardLink is stretched in case user accidentally throws in more of them. But 🤷🏻‍♂️

packages/web/src/scss/components/Card/_CardMedia.scss Outdated Show resolved Hide resolved

color: theme.$heading-color;

&:not(:last-child) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tools.add-bottom-spacing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe not. The mixin is originally intended to separate direct children of Card with a unified gap. It's too generic, maybe I should better get rid of it…

packages/web/src/scss/components/Card/_theme.scss Outdated Show resolved Hide resolved
<!-- End user content -->
</div>
<footer class="CardActions CardActions--alignmentXLeft">
<a href="#" class="Button Button--medium Button--primary">Primary</a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you use a button somewhere so we can see it works correctly?

@crishpeen
Copy link
Member

crishpeen commented Nov 19, 2024

Horizontal cards do not fit on mobile screen :(

</footer>
</article>

<!-- TODO: Do we want to present the auto size in horizontal layout? The image is huge… -->
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What shall we do with this? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants