Add new Card component #418
Conversation
😅 |
|
I can try to review the code on its own, but it will be super helpful to have at least some sort of idea for what the spec for this new data might be and ideally some example test data would be awesome. |
| }, | ||
| url: { | ||
| type: String, | ||
| required: false, |
There was a problem hiding this comment.
This prop type seems slightly incompatible with the Reference.url prop type which always requires a URL. Is this truly optional and if so, do we need to make other changes to accommodate that?
There was a problem hiding this comment.
I think if you pass an empty url it should just render an inactive Reference. I will add a default state so we dont have undefined and potential errors from Vue.
| @@ -190,4 +190,9 @@ | |||
| --color-tutorial-hero-text: #{dark-color(figure-gray)}; | |||
| --color-tutorial-hero-background: #{dark-color(fill)}; | |||
| --color-navigator-item-hover: #{change-color(light-color(fill-blue), $alpha: 0.05)}; | |||
|
|
|||
| --color-card-background: var(--color-fill); | |||
There was a problem hiding this comment.
This is a little hard to say without some example data, but is it an expected scenario to have a card without an image? Depending on how common that scenario is, we might want to consider eliminating special color values for them to avoid bloating the very large number of special colors we already have. Ignoring the shadow, the only benefit to including these new variables would be so that these could potentially be specialized on their own through theming, right?
Not necessarily advocating for one approach of the other yet—just making sure we've thought through the common use case before we add new global color variables.
dobromir-hristov
force-pushed
the
dhristov/r97714707-introduce-card-component
branch
from
00de731 to
026e400
Compare
|
Hey I took into account both of your feedback. You can give it another look. |
|
Not totally sure how I should be testing this. |
This is part of #419, unfortunately as of now, there isnt an isolated way to test this specific PR, unless we build a Storybook-like page with example components. |
dobromir-hristov
force-pushed
the
dhristov/r97714707-introduce-card-component
branch
from
c894277 to
24ac1cf
Compare
|
@swift-ci test |
|
@marinaaisa please review again, I have updated the code to fit both your comments. |
|
@swift-ci test |
dobromir-hristov
deleted the
dhristov/r97714707-introduce-card-component
branch
Bug/issue #, if applicable: 97714707
Summary
This introduces a new Card component, that will be used as a building block for TopicSectionsStyle and Links.
Most props are optional, and the cards self-adjust, so they can act as simple or detailed as need be.
Dependencies
@TopicsVisualStyle#419 - This PR is the main user of the Card for now.Testing
NA
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
npm test, and it succeeded