-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat(PosterCard): allow backgroundColor/variant #841
Changes from 10 commits
3a62201
ca8c627
e1c2498
8a39686
06ef0fc
73c27fb
2a9a62a
05c0337
25ca481
ae9fb8a
757bad4
a24796c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
yceballost marked this conversation as resolved.
Show resolved
Hide resolved
|
atabel marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,11 @@ import Image from '../image'; | |
onPress={() => {}} | ||
trackingEvent={{name: 'do-something'}} | ||
/>; | ||
<PosterCard title="title" isInverse />; | ||
<PosterCard title="title" variant="inverse" />; | ||
<PosterCard title="title" variant="alternative" />; | ||
<PosterCard title="title" isInverse backgroundColor="red" />; | ||
<PosterCard title="title" variant="inverse" backgroundColor="red" />; | ||
Comment on lines
+34
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these are valid usage examples |
||
|
||
// @ts-expect-error onPress and href can't be used together | ||
<PosterCard title="title" onPress={() => {}} href="/" />; | ||
|
@@ -42,8 +47,14 @@ import Image from '../image'; | |
<PosterCard title="title" trackingEvent={{name: 'do-something'}} />; | ||
// @ts-expect-error backgroundImage and backgroundVideo can't be used together | ||
<PosterCard title="title" backgroundImage="" backgroundVideo="" />; | ||
// @ts-expect-error backgroundImage or backgroundVideo are mandatory | ||
// @ts-expect-error backgroundImage and backgroundColor can't be used together | ||
<PosterCard title="title" backgroundImage="" backgroundColor="" />; | ||
// @ts-expect-error backgroundVideo and backgroundColor can't be used together | ||
<PosterCard title="title" backgroundVideo="" backgroundColor="" />; | ||
// @ts-expect-error backtroundColor, backgroundImage or backgroundVideo are mandatory | ||
atabel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
<PosterCard title="title" />; | ||
// @ts-expect-error if you set a custom backgroundColor, you should specify the variant | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not using a default value for the variant if it's not provided? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I prefer this to be explicit. You set either |
||
<PosterCard title="title" backgroundColor="red" />; | ||
|
||
(isTouchable: boolean) => <SnapCard title="title" href={isTouchable ? '/' : undefined} />; | ||
(isTouchable: boolean) => <SnapCard title="title" to={isTouchable ? '/' : undefined} />; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it should be fixed now There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
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 card is impossible to have in dark mode. ThemeVariant is forcing these text to white :( I think that could be a problem. Can we force themeVariant also in dark mode?
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 is tricky because Text just uses textPrimary/textPrimaryInverse depending on the variant context, but those color constants are both white in dark mode. So the only way to fix this would be to find a way to disable dark mode in this area. We could explore a solution in a different PR to manage this case
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.
https://jira.tid.es/browse/WEB-1502