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

server: fix blurry PWA icon #465

Merged
merged 1 commit into from
Oct 13, 2024
Merged

Conversation

quantum5
Copy link

@quantum5 quantum5 commented Oct 8, 2024

I tried creating the server version as a PWA with Chrome on Android, but the icon is very blurry. My solution is to create a PNG of the icon and make the manifest.webmanifest use that instead of favicon.ico, which seems to have fixed the problem.

@rpg2014
Copy link

rpg2014 commented Oct 8, 2024

Thanks for this fix, this has been bugging me for a while. I see that chrome is complaining that a png, svg, or webp icons isn't specified in the manifest on my server's version (0.63.3).
Image:
Chrome warning

I'm not super familiar with this project, but I made the same changes as this commit on my instance and it worked correctly.

Copy link

@rpg2014 rpg2014 left a comment

Choose a reason for hiding this comment

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

Testing these changes locally and they work as expected. Not 100% sure if this is the implementation the maintainers would want to go with. I could see an argument around setting up a more generic express route that handles any files put into the public folder, rather than an route specific to icon.png

@@ -47,6 +47,7 @@ app.use(cookieParser());
app.use(express.static(path.join(scriptDir, 'public/root')));
app.use(`/manifest.webmanifest`, express.static(path.join(scriptDir, 'public/manifest.webmanifest')));
app.use(`/robots.txt`, express.static(path.join(scriptDir, 'public/robots.txt')));
app.use(`/icon.png`, express.static(path.join(scriptDir, 'public/icon.png')));
Copy link

@rpg2014 rpg2014 Oct 9, 2024

Choose a reason for hiding this comment

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

You should probably use the png that already exists in the images folder instead of replicating the image in the public folder. https://github.com/TriliumNext/Notes/blob/develop/images/app-icons/png/512x512.png

Copy link
Author

Choose a reason for hiding this comment

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

Good find. I did a quick find and didn't see it, so I generated my own. But this is so much better.

Copy link
Author

Choose a reason for hiding this comment

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

Ah I see, this file is only on the develop branch and not the last release or master branch.

Copy link
Author

@quantum5 quantum5 left a comment

Choose a reason for hiding this comment

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

Testing these changes locally and they work as expected. Not 100% sure if this is the implementation the maintainers would want to go with. I could see an argument around setting up a more generic express route that handles any files put into the public folder, rather than an route specific to icon.png

Yeah, I'll let the maintainers decide what the best way to serve that asset is. For now, I'll just get rid of the separate file.

@@ -47,6 +47,7 @@ app.use(cookieParser());
app.use(express.static(path.join(scriptDir, 'public/root')));
app.use(`/manifest.webmanifest`, express.static(path.join(scriptDir, 'public/manifest.webmanifest')));
app.use(`/robots.txt`, express.static(path.join(scriptDir, 'public/robots.txt')));
app.use(`/icon.png`, express.static(path.join(scriptDir, 'public/icon.png')));
Copy link
Author

Choose a reason for hiding this comment

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

Good find. I did a quick find and didn't see it, so I generated my own. But this is so much better.

Copy link

@eliandoran eliandoran left a comment

Choose a reason for hiding this comment

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

LGTM.

@eliandoran eliandoran merged commit 8136a29 into TriliumNext:develop Oct 13, 2024
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.

3 participants