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: added PWA asset generation #110

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

knownotunknown
Copy link

@knownotunknown knownotunknown commented Nov 4, 2024

Features

CI

  • Excluded PWA assets from bundle size calculation

PR 1 of 2 to close #59.

2nd PR will include caching via service worker and testing via Lighthouse GitHub Actions. Could split up caching and testing if needed, but not sure if that's worth it/makes sense.

Original, full PR: #108

index.html Outdated Show resolved Hide resolved
public/manifest.json Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Nov 4, 2024

deployed preview: https://110.connect-d5y.pages.dev

Welcome to new-connect! Make sure to:

  • read the contributing guidelines
  • mark your PR as a draft until it's ready to review
  • post the preview on Discord; feedback from users will speedup the PR review

Mobile

Desktop

@@ -11,6 +11,9 @@
"scripts": {
"build": "vite build",
"dev": "vite",
"generate-pwa-assets": "npx pwa-asset-generator ./public/images/icon-vector.svg ./pwa-assets -m ./public/manifest.json -i ./index.html --sizes \"72x72,96x96,128x128,144x144,152x152,192x192,384x384,512x512\" --quality 75 --background \"#ffffff\" --padding \"10%\" --platform android,ios --opaque false",
"move-pwa-assets": "rm -rf ./public/pwa-assets && mv ./pwa-assets ./public/",
"pwa-setup": "npm run generate-pwa-assets && npm run move-pwa-assets",
Copy link
Author

@knownotunknown knownotunknown Nov 4, 2024

Choose a reason for hiding this comment

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

  1. "generate-pwa-assets":

    • Takes our vector icon from public/images/icon-vector.svg
    • Generates various sizes of icons required for PWA (72px to 512px)
    • Updates manifest.json and index.html with the new icons
    • Applies settings:
      • 75% image quality for bundle size optimization
      • 10% padding around icons
      • Compatible with both Android and iOS
      • Transparent background allowed
  2. "move-pwa-assets":

    • Cleans up old assets to avoid duplicates
    • Moves generated assets to the public directory
  3. "pwa-setup":

    • Runs both commands in sequence to generate and organize PWA assets

index.html Outdated
Copy link
Author

Choose a reason for hiding this comment

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

All changes in this file are auto-generated

Copy link
Author

Choose a reason for hiding this comment

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

All changes in this file are auto-generated

Copy link
Contributor

Choose a reason for hiding this comment

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

is the whole file auto generated?

Copy link
Author

Choose a reason for hiding this comment

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

No, only the changes shown.

@knownotunknown knownotunknown marked this pull request as ready for review November 4, 2024 23:49
Copy link
Contributor

@adeebshihadeh adeebshihadeh left a comment

Choose a reason for hiding this comment

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

Can we gitignore the generated assets and just generate them at build-time?

@@ -11,6 +11,9 @@
"scripts": {
"build": "vite build",
"dev": "vite",
"generate-pwa-assets": "npx pwa-asset-generator ./public/images/icon-vector.svg ./pwa-assets -m ./public/manifest.json -i ./index.html --sizes \"72x72,96x96,128x128,144x144,152x152,192x192,384x384,512x512\" --quality 75 --padding \"10%\" --platform android,ios --opaque false",
Copy link
Contributor

Choose a reason for hiding this comment

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

let's document these in the README?

Copy link
Contributor

Choose a reason for hiding this comment

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

is the whole file auto generated?

@knownotunknown
Copy link
Author

Can we gitignore the generated assets and just generate them at build-time?

Sure. I went ahead and created separate build and build:prod commands since generating pwa-assets increases total build time from 1s to 5s, and you only need to generate pwa-assets once anyways.

@adeebshihadeh
Copy link
Contributor

Sure. I went ahead and created separate build and build:prod commands since generating pwa-assets increases total build time from 1s to 5s, and you only need to generate pwa-assets once anyways.

This is not a good tradeoff. What happens if the assets get updated? How do I discover this build:prod command? It should just be part of the normal build. One case is 10x better than two cases.

@knownotunknown
Copy link
Author

knownotunknown commented Nov 15, 2024

Sure. I went ahead and created separate build and build:prod commands since generating pwa-assets increases total build time from 1s to 5s, and you only need to generate pwa-assets once anyways.

This is not a good tradeoff. What happens if the assets get updated? How do I discover this build:prod command? It should just be part of the normal build. One case is 10x better than two cases.

That's fair. Fixed. I wasn't sure about the reasoning for .gitignoring the pwa-assets since those assets would probably rarely change anyways, and it added an extra build step, so I thought we were really just optimizing for cleanliness.

Was the main motivation of .gitignoring to keep our assets up-to-date with the source svg?

@adeebshihadeh
Copy link
Contributor

adeebshihadeh commented Nov 15, 2024

Yes, this way they're never out of date and the SVG is the source of truth. Generally, you don't want to check in build products.

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.

Implement PWA features
2 participants