-
Notifications
You must be signed in to change notification settings - Fork 3
make primary buttons shadowy, remove edge=none #1213
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
Conversation
| }, | ||
| edge: { | ||
| options: ["circular", "rounded", "none"], | ||
| options: ["circular", "rounded"], |
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.
edge="none" is not in figma; it was added for resource cards, but that is handled a little differently now (changed in #1180 )
| hasBorder && { | ||
| backgroundColor: "transparent", | ||
| borderColor: "currentcolor", | ||
| borderStyle: variant === "secondary" ? "solid" : "none", |
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.
Previously, const hasBorder = variant === "secondary" || variant === "text". But that was weird because text variant doesn't actually have a border. It was just set that way to pick up backgroundColor: transparent.
Applying backgroundColor: transparent to the text variant separately is simpler / more understandable, imo.
| "Without Image": makeResource({ | ||
| "Without Image": { | ||
| ...makeResource(), | ||
| image: null, |
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 change is unrelated, but was something I did while opening
Decided to keep it.
22adcae to
c917dca
Compare
abeglova
left a comment
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.
👍
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/4700
Description (What does it do?)
Makes primary buttons (and only primary buttons) have a shadow to match figma.
Screenshots (if appropriate):
App There aren't too many primary buttons in the app so far.

Storybook:

How can this be tested?
yarn storybook) and check:Buttons should be unchanged, except primary buttons should have a shadow AND only in their default state (no shadow on hover).