Story: Basic Card Component#2107
Conversation
|
@gregjkal For this one, let me know if you have any feedback on making this more extensible/reusable. I'd like this to be something we can build off of with minimal copy pasting |
gregjkal
left a comment
gregjkal
left a comment
There was a problem hiding this comment.
Not a bad start.
For extensibility... I don't think our cards have enough in common besides styling to make this inheritable / extensible. I do think we would benefit from a common .card class and css rules, since there is a lot of commonality there. Something like:
.card {
font-family: var(--font-sans);
display: flex;
flex-direction: column;
max-width: 458px;
width: 100%;
box-sizing: border-box;
border-radius: var(--border-radius-xl, 12px);
border: 1px solid var(--color-stroke-weak, rgba(5, 8, 22, 0.1));
background: var(--color-surface-weak, white);
overflow: hidden;
}Then all our card components would have classes like
<div class="card basic-card">| {% block bottom_hr %} | ||
| <hr class="card__hr" /> | ||
| {% endblock %} | ||
| {% block buttons %} |
There was a problem hiding this comment.
Let's not roll our own buttons here - re-use the button components.
There was a problem hiding this comment.
Having said this, I'm realizing none of the other components use button components 🥴.
Let's start fixing that with this PR 😊
| {% block body_content %} | ||
| <span class="card__text">{{ text }}</span> | ||
| {% endblock %} | ||
| </div> |
There was a problem hiding this comment.
Bug: card__text class is used here but never defined in card.css. This text will be unstyled.
There was a problem hiding this comment.
So on second review of the figma, I don't believe there is any actual styling on this text, so eliminating this class for now
gregjkal
left a comment
gregjkal
left a comment
There was a problem hiding this comment.
Thanks for addressing all the feedback. Couple very minor new things, but approving as-is.
4 participants
Uh oh!
There was an error while loading. Please reload this page.