Skip to content
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

Merged
merged 4 commits into from
Nov 25, 2023

Conversation

hashkar123
Copy link
Contributor

@hashkar123 hashkar123 commented Nov 19, 2023

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:

  • Tested the feature and it's working on a current and clean install.
  • Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)
  • Created / Updated Tests (If necessary)
  • Created / Updated documentation (If necessary)

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.
@hashkar123 hashkar123 changed the title Add logo support in GamePage and GamePicture Fix Issue #2466 GamePage image is missing logo when available Nov 19, 2023
@@ -231,6 231,7 @@ export default React.memo(function GamePage(): JSX.Element | null {
art_square,
art_cover,
art_background,
art_logo: logo = undefined,
Copy link
Collaborator

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

Copy link
Collaborator

@arielj arielj Nov 21, 2023

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
Copy link
Collaborator

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,
Copy link
Collaborator

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}
/>
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these { ... } needed?

@hashkar123
Copy link
Contributor Author

hashkar123 commented Nov 22, 2023

Thanks for the review!
I'll get these fixed tomorrow or the day after it.

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_...'
@arielj
Copy link
Collaborator

arielj commented Nov 24, 2023

I'm testing this and it works fine, but I think the logo is too big compared to the other places we do a similar thing:

In the library it looks like this:
image

In the old design it looks like this:
image

In the new design it looks like this:
image

I think the old CSS that was there is not used by anything else. I played a bit with it and with:

/* in src/frontend/screens/Game/GamePicture/index.css */
.gamePicture > .gameLogo {
  position: absolute;
  bottom: 50%;
  transform: translateY(50%);
  width: 40%;
  min-width: 140px;
}

it looks like this:
image

image

which looks more consistent with the library

@hashkar123
Copy link
Contributor Author

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.
Do you want me to make the logo look the same as in the library? It just wasn't clear if you wanted me to change that or not.

@arielj
Copy link
Collaborator

arielj commented Nov 24, 2023

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. Do you want me to make the logo look the same as in the library? It just wasn't clear if you wanted me to change that or not.

yes please do that change, I think it looks better to make it more consistent

Copy link
Collaborator

@arielj arielj left a 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!

@arielj arielj merged commit 177d480 into Heroic-Games-Launcher:main Nov 25, 2023
13 checks passed
@hashkar123 hashkar123 deleted the gamepage-no-logo-2466 branch November 25, 2023 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants