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

Add basic PWA support #2405

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

Conversation

alistair3149
Copy link

@alistair3149 alistair3149 commented Oct 30, 2022

Changes

Should be enough to fulfill the installability requirements (#181):

  • Add webapp manfiest
  • Add basic service worker
    • Does nothing currently, but it is enough to fulfill the installability requirements
  • Add square icons per PWA requirements (192px, 512px, SVG)
    • Icons are based on this vector icon
    • PNGs are optimized with PNGCrush
    • SVG is optimized with SVGO
    • Add maskable icons for fancier app icons

Tests

  • This PR does not require tests

Changelog

  • Entry has been added to changelog

Documentation

  • This change does not need a documentation update

Dark mode

  • This PR does not change the UI

Notes

There are still some potential PWA improvements can be added in the future, but sadly I don't know enough about Plausible or Elixir to add those myself. Maybe someone can take a look:

On an unrelated note, I realized that the original SVG icon has some dirty pathing where the "P" icon does not line up with the circle. Not sure if it is intended so I left that there. The PNG icons in the static folder can be further reduced in file sizes as well to save some bandwidth.

This is my first time committing a PR here, please let me know if there is anything you need!

This should be enough to fulfill the instability requirements
* Add webapp manfiest
* Add basic service worker
** It does nothing currently, but it is enough to fulfill the instability requirements
* Add square icons per PWA reqirements (192px, 512px, SVG)
** PNGs are optimized with PNGCrush
** SVG is optimized with SVGO
** Add maskable icons for fanicer app icons
@ukutaht
Copy link
Contributor

ukutaht commented Oct 31, 2022

Thanks @alistair3149 !

Did you see this open PR? #1981

How would you say your PR compares to the older one?

@alistair3149
Copy link
Author

alistair3149 commented Oct 31, 2022

The older PR is incomplete and this PR covers everything in the previous PR.

  • In the older PR, the service worker is loaded directly from the HEAD element without compatibility check. It can throw error on browsers that don't support service worker.
  • In the older PR, the service worker script can only access the /js/ subdirectory. That means it cannot cache most assets, which makes it not ideal. This PR places the service worker in domain root instead, allowing it to access all assets for caching.
  • In the older PR, there is only one icon defined in the manifest, and it is a rectangle icon (150px x 200px). Some devices require square icon and specific dimensions to function. This PR uses square icons with dimensions recommended for Chromium, vector icon, and also maskable icons to cover most use cases.
  • The older PR is missing some property in the manifest. You can compare the two here: Old vs New
  • The older PR uses manifest.json as the file name, which is an older standard. The current best practice is to use .webmanifest
  • The older PR has a shortcut to the /create page. I didn't add any shortcut in this PR because I'm not sure about the idea of adding pages that has access restrictions (e.g. /register might be disabled on some sites)

@ukutaht
Copy link
Contributor

ukutaht commented Nov 1, 2022

Cheers thanks. I'll close the old one and keep this one as the one to consider.

@alistair3149
Copy link
Author

Is there anything I can help to move this forward?

@ukutaht
Copy link
Contributor

ukutaht commented Nov 14, 2022

@alistair3149

Each PR needs to be reviewed, tested by the core team, and integrated properly. This is not currently high on our list of priorities. For features/PRs that haven't been agreed on with the core team in advance, we can't guarantee any response times.

We do want to get the list of PRs down so I might put this on our backlog sometime soon. But no promises, sorry! I really do appreciate the effort put in by all external contributors, but I hope you understand that it's the core team's responsibility to uphold quality standards. Integrating PRs takes time and effort from our side (not just on the contributor's side) and needs to be considered within the context of all our other priorities.

@alistair3149
Copy link
Author

No problem and appreciate the reply! I just want to make sure that all work has been done and it is ready for handoff. Hope to see it integrated soon!

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

None yet

4 participants