-
Notifications
You must be signed in to change notification settings - Fork 4
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
Modify Card component to accept header #417
Conversation
✅ Deploy Preview for vip-design-system-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
db64c3c
to
12117e1
Compare
294f631
to
c4a6d7e
Compare
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.
First of all, thanks for creating the component here first! This is really important.
I would like to propose a different a approach from what you started, if you don't mind. The goal is to keep thins standard with the way we've be doing things.
1 - I believe the BoxWithHeading could easily be a variation of "Card". You could add a header property accepting a text or another component, and simply use the Card.
2 - The way you're customizing the variables is a little bit different of what we do usually. It might look not so "flexible", but to keep the design aligned with Figma, we usually work with "variants" of a component. Instead of passing the header bg color and header text color, you can pass the variant="primary"
for example. The primary variant will have a "static" variation of the header you want to use.
If I may suggest what it would like in my mind, is this:
<Card>
<Card.Header>This is my header</Card.Header>
<Card.Body>This is my body</Card.Body> // this is optional
</Card>
You can take a look of a variant example here: src/system/Button/Button.tsx
3 - If you go with the Card
idea, can you move this stories.tsx documentation and write some documentation? I can help too.
4 - We have a set of border colors, you might want to try borders.2
or something similar. Figma should show the correct set of colors.
c4a6d7e
to
d2a35c2
Compare
25699b5
to
59335e8
Compare
29c120b
to
093578b
Compare
093578b
to
21fca0f
Compare
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.
Thanks! I left some things, but it's in a much better shape now!
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.
Amazing! :D
Thanks for your patience!
Description
Modify existing Card component to accept header property:
Checklist
Steps to Test
Outline the steps to test and verify the PR here.
Example:
npm run dev
.