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

feat: replace blurry png logo with svg #904

Merged
merged 3 commits into from
Sep 30, 2024

Conversation

edeuss
Copy link

@edeuss edeuss commented Sep 26, 2024

Upped the resolution to the same as the Jellyfin icon in the same folder.

Might be a good idea to switch to use the SVG instead of an PNG.

@Chaphasilor
Copy link
Collaborator

Thanks for the PR! We already have an SVG of the logo, should we just switch to using that right now? ^^

@edeuss
Copy link
Author

edeuss commented Sep 27, 2024

@Chaphasilor Switched to use an SVG, Keeping the image commit in the PR for now (looks like the image is used in other places too. I can switch the other places to use the SVG in another PR). Or I can make it apart of this one, thoughts?

@edeuss
Copy link
Author

edeuss commented Sep 27, 2024

Looking in the code it looks like there is only two other locations the image is used, the settings page and the drawer.

@Chaphasilor
Copy link
Collaborator

I'm a fan of SVG, if we can use it in more places that would be even nicer.
But I don't have a lot of experience with using SVG in Flutter, how's the compatibility across different platforms? Does it look correctly everywhere?

@edeuss
Copy link
Author

edeuss commented Sep 27, 2024

@Chaphasilor Added the last two pages to the PR, I have never had any real issues with SVG's on any of Flutter's supported platforms.

@edeuss edeuss changed the title fix: blurry login logo feat: replace blurry png logo with svg Sep 27, 2024
@Chaphasilor
Copy link
Collaborator

Got it. Is there any particular reason for creating a new SVG? Was the existing one in a wrong location or had too much padding?

@edeuss
Copy link
Author

edeuss commented Sep 30, 2024

@Chaphasilor I did that because I wanted to keep everything imported assets in images (since you guys don't seem to use the assets folder for any app assets, confused me a little bit. Should really be the other way around).

@Chaphasilor
Copy link
Collaborator

Ah yeah that tracks. I have no idea why it was done that way, but I'm open to restructuring things if that makes more sense! Applies to the entire project, if there's something odd that we could do better, we should do that :)

@edeuss
Copy link
Author

edeuss commented Sep 30, 2024

@Chaphasilor We could do that in a later pr. But I think this PR is good.

@Chaphasilor Chaphasilor merged commit 8b007b2 into jmshrv:redesign Sep 30, 2024
4 checks passed
@Chaphasilor
Copy link
Collaborator

Sounds good. Let's do it like that. Thanks for the PR!

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