Skip to content

favicon: improve PWA and cross-platform icon support #1186

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Optimus-NP
Copy link
Contributor

@Optimus-NP Optimus-NP commented Mar 9, 2025

Fixes: #1189

Add proper configuration for Progressive Web App (PWA) and platform-specific icons across the application. Changes include:

  • Update site.webmanifest with correct icon purposes for PWA
  • Add Android Chrome icons (192x192, 512x512) to HTML
  • Add Windows tile image configuration
  • Ensure consistency between JS and no-JS pages

This ensures proper icon display across all platforms while following platform-specific best practices for icon implementation.

@Optimus-NP
Copy link
Contributor Author

Hi @kelson42 @veloman-yunkan

Requesting you to review this PR.

@Optimus-NP
Copy link
Contributor Author

Hi @rgaudin @veloman-yunkan @kelson42

Requesting you to review this PR.

@rgaudin rgaudin requested review from veloman-yunkan and juuz0 March 17, 2025 08:24
@rgaudin
Copy link
Member

rgaudin commented Mar 17, 2025

Looks OK ; requested review from maintainers

@rgaudin
Copy link
Member

rgaudin commented Mar 17, 2025

I don't think it fixes said ticket though…

@Optimus-NP
Copy link
Contributor Author

@veloman-yunkan

Please help in reviewing this PR.

@kelson42
Copy link
Collaborator

This does not fix #716 AFAIZk

@kelson42 kelson42 force-pushed the kiwix-tools_issues-716 branch from 7052b24 to 1196a7c Compare March 17, 2025 19:39
@kelson42 kelson42 self-requested a review March 17, 2025 19:40
Copy link

codecov bot commented Mar 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.45%. Comparing base (ad58a50) to head (1196a7c).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1186   +/-   ##
=======================================
  Coverage   41.45%   41.45%           
=======================================
  Files          59       59           
  Lines        4289     4289           
  Branches     2349     2349           
=======================================
  Hits         1778     1778           
  Misses        999      999           
  Partials     1512     1512           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Optimus-NP
Copy link
Contributor Author

This does not fix #716 AFAIZk

Thank you for the review. I have updated the description that it Fixes: #1189

Please take a look and merge this PR.

@rgaudin
Copy link
Member

rgaudin commented Mar 19, 2025

First item in the ticket is

Enhance favicon assets to support high-DPI screens.

There is no asset change in the PR.

I am using this PR as an opportunity to discuss kiwix-serve's Icon. It is currently using the Kiwix bird (we might want to check that it's the proper one – margin wise). While it would have already been possible for browsers (especially on mobile) to save this as an app, this new manifest makes it explicit. Is this bird-only icon what we for for a PWA of kiwix-serve? @kelson42 ?

@Optimus-NP
Copy link
Contributor Author

@rgaudin @kelson42

I’d love to get this PR merged. Please let me know if there's anything pending on my end, I'd be happy to work on it.

Add proper configuration for Progressive Web App (PWA) and platform-specific icons across the application. Changes include:

- Update site.webmanifest with correct icon purposes for PWA
- Add Android Chrome icons (192x192, 512x512) to HTML
- Add Windows tile image configuration
- Ensure consistency between JS and no-JS pages

This ensures proper icon display across all platforms while following platform-specific best practices for icon implementation.
@kelson42 kelson42 force-pushed the kiwix-tools_issues-716 branch from 1196a7c to c8bce1d Compare April 20, 2025 13:40
@kelson42
Copy link
Collaborator

@Optimus-NP Thank you for your PR... and your patience. I have rebased it on the latest version of main. See please my comment on the #1189, I would like first to clarify what is not OK with the current code before assessing your PR.

@rgaudin
Copy link
Member

rgaudin commented Apr 21, 2025

@rgaudin @kelson42

I’d love to get this PR merged. Please let me know if there's anything pending on my end, I'd be happy to work on it.

Maybe answering my comment?

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.

Deliver Proper Favicon Support for High-DPI Screens in Kiwix Server
3 participants