-
-
Notifications
You must be signed in to change notification settings - Fork 450
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
Fix Issue #2466 GamePage image is missing logo when available #3245
Fix Issue #2466 GamePage image is missing logo when available #3245
Conversation
In GamePage component, add logo variable and pass it to GamePicture component. In GamePicture component, add logo parameter; add logo element (same as the CachedImage logo element in GameCard) There was no need to add CSS, since there was already CSS for logo in GamePicture that positions it at the bottom center.
@@ -231,6 231,7 @@ export default React.memo(function GamePage(): JSX.Element | null { | |||
art_square, | |||
art_cover, | |||
art_background, | |||
art_logo: logo = 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.
maybe we can just use art_logo
everywhere? it makes it easier to understand that it's the same thing that comes from the gameInfo and we already use art_...
for the other images, makes it more consistent
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.
also, I think there's no need to do the = undefined
since it will be undefined if it is undefined anyway
I see we have this in GameCard, but in that component we do rename other properties like cover or appName and I think we can ignore the = undefined
there too (not something to change in this PR though)
@@ -6,10 6,17 @@ import fallbackImage from 'frontend/assets/heroic_card.jpg' | |||
|
|||
interface Props extends React.ImgHTMLAttributes<HTMLImageElement> { | |||
art_square: string | |||
logo?: string | 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.
do we need the | undefined
? I think because it's optional with the ?
it already includes undefined as a possibility
function GamePicture({ art_square, store, className, ...props }: Props) { | ||
function GamePicture({ | ||
art_square, | ||
logo = 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.
same here as the other places, I'd just call this art_logo
and without the default undefined
, makes naming more consistent with the other places and it's undefined if not defined already
fallback={fallback} | ||
{...props} | ||
/> | ||
{ |
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.
are these { ... }
needed?
Thanks for the review! I'll make sure to check everything you mentioned :D |
Purpose of this is to maintain naming consistency with other variables, such as 'art_cover' or other 'art_...'
I also think the logo looked weird in the game page. It would be less weird if it was more consistent with the Library's logo. I could do that. |
yes please do that change, I think it looks better to make it more consistent |
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.
Looks great now, thanks!
Fix Issue #2466 GamePage image is missing logo when available
Add logo support in GamePage and GamePicture.
In GamePage component, add logo variable and pass it to GamePicture component. In GamePicture component, add logo parameter; add logo element (same as the CachedImage logo element in GameCard)
There was no need to add CSS, since there was already CSS for logo in GamePicture that positions it at the bottom center.
Use the following Checklist if you have changed something on the Backend or Frontend: