-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add Cards component to style guide #35
Conversation
@@ -23,5 +23,6 @@ | |||
<div class="w-f192 bg-teal">f192 - 192px</div> | |||
<div class="w-f224 bg-teal">f224 - 224px</div> | |||
<div class="w-f256 bg-teal">f256 - 256px</div> | |||
<div class="w-f320 bg-teal">f320 - 320px</div> |
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.
This was a width specified in the Figma design so I added it.
export let icon = UserFilled; | ||
</script> | ||
|
||
<div class="p-6 card-shadow rounded-xl bg-{bgColor} {$$restProps.class}"> |
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.
<div class="p-6 card-shadow rounded-xl bg-{bgColor} {$$restProps.class}"> | |
<div class="p-6 card-shadow rounded-xl bg-cream {$$restProps.class}"> |
Thinking we degault to cream, then when a user wants to change the color, they can just set the class to the new color that they want and it can override the cream through the restProps.class.
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.
I did it this way because the background color as a prop was part of the Issue specification. It does already default to cream, however, I think it's not the best practice to have two background color classes end up in the class list.
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.
A couple of suggestions, but overall looking good!
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.
- Pulled branch, ran storybook
- Test drove new Card component
Looks great!
Co-authored-by: V. Claire Olmstead <43625033+claireolmstead@users.noreply.github.com>
Co-authored-by: V. Claire Olmstead <43625033+claireolmstead@users.noreply.github.com>
fafce09
to
cec63f0
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.
👍🏼
Goal
The purpose of this PR is to add a Card component according to the UX designs.
The Card takes as props
When I changed the bgcolor it seemed odd not to let you change the text color. Since the title text color was different from the card text color, it also seemed you should be able to specify that color as well. The defaults match what is in the design.
Closes #9
Steps to Verify:
npm run storybook
Screenshots (optional):