-
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
PosterCard-background #844
Changes from 7 commits
7e3154c
3dfa669
c2ef620
08c4498
8698f8b
c381da8
b925ba3
43028a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1184,14 +1184,16 @@ interface PosterCardBaseProps { | |
titleLinesMax?: number; | ||
description?: string; | ||
descriptionLinesMax?: number; | ||
isInverse?: boolean; | ||
backgroundColor?: string; | ||
} | ||
|
||
interface PosterCardWithImageProps extends PosterCardBaseProps { | ||
backgroundImage: string; | ||
backgroundImage?: string; | ||
} | ||
|
||
type PosterCardWithVideoProps = Omit<PosterCardBaseProps, 'actions' | 'onClose'> & { | ||
backgroundVideo: VideoSource; | ||
backgroundVideo?: VideoSource; | ||
poster?: string; | ||
backgroundVideoRef?: React.RefObject<VideoElement>; | ||
}; | ||
|
@@ -1223,6 +1225,8 @@ export const PosterCard = React.forwardRef<HTMLDivElement, PosterCardProps>( | |
titleLinesMax, | ||
description, | ||
descriptionLinesMax, | ||
isInverse = true, | ||
backgroundColor, | ||
...touchableProps | ||
}, | ||
ref | ||
|
@@ -1258,7 +1262,7 @@ export const PosterCard = React.forwardRef<HTMLDivElement, PosterCardProps>( | |
className={styles.boxed} | ||
width="100%" | ||
minHeight="100%" | ||
isInverse | ||
isInverse={isInverse} | ||
background={ | ||
backgroundImage || backgroundVideo | ||
? isExternalInverse | ||
|
@@ -1275,7 +1279,10 @@ export const PosterCard = React.forwardRef<HTMLDivElement, PosterCardProps>( | |
> | ||
{isTouchable && <div className={styles.touchableCardOverlayMedia} />} | ||
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 did not understand your comment. Could you explain it better? 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. Of course! there are three cases:
The same applies to the top actions container (image posted in the previous comment), depending of the case the container will have different default an interaction states.
I hope the explanation is more clear now 😃 |
||
|
||
<div className={styles.displayCardContainer}> | ||
<div | ||
className={styles.displayCardContainer} | ||
style={backgroundColor ? {background: backgroundColor} : undefined} | ||
> | ||
<ThemeVariant isInverse={isExternalInverse}> | ||
<div className={styles.displayCardBackground}> | ||
{backgroundVideo ? video : backgroundImage ? image : undefined} | ||
|
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 usage is wrong
https://tinyurl.com/24fsdre5
the possibility to change text color its only allowed for custom background color
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.
update: In windows works (or isInverse doesn't works xD) as expected. In Mac OS chrome this is happening
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.
"the possibility to change text color its only allowed for custom background color"
The text should only change the color if it receives a background color, in the case of an image, should the color be default?
"update: In windows works (or isInverse doesn't works xD) as expected. In Mac OS chrome this is happening"
I did not understand your comment. Can you explain better?
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.
Exactly. When the card has an image, the gradient covers the need to make the text visible in most of the images. So we don't need to give the possibility to make color changes in the background image and video cases
Sure! I tried the link from my first message in Windows, and the isInverse prop doesn't works in the same way as in Chrome in Mac OS. Quite weird in my opinion.. i don't know if anybody can check this in other computes with both OS.In chrome window 10, isInverse prop always paint the text in white (with true and false value) and in chrome in Mac OS, the prop change color as I show in the image 🤔Do I make myself clear?Edited: Talked with edu, the reason was dark mode