-
-
Notifications
You must be signed in to change notification settings - Fork 585
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: pwa with push notifications #337
Conversation
|
✅ Deploy Preview for elk-zone ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
# Conflicts: # composables/users.ts # package.json # pnpm-lock.yaml
isSupported, | ||
notificationPermission, | ||
subscribe, | ||
unsubscribe, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to add ui stuff for unused stuff...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sxzz added unused stuff logic
…/feat-pwa # Conflicts: # modules/pwa/runtime/sw.ts
Some UI thoughts:
Reworked the pr to hopefully remove the |
(FF was my error - it's working fine.) |
About the settings panel, we need to review it. I just add the checks to test masto api (beware, on change will go to the server) and disable button to test unsubscribe from push notifications. Push notifications works also on Safari 16, cyberalien did some tests. |
Thx ❤️ |
@danielroe latest changes:
|
} | ||
else { | ||
// Something went wrong, try to subscribe again | ||
return unsubscribe({ registration, subscription }).then( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should:
- unscrubscribe/remove push subscription from backend:
useMasto().pushSubscriptions.remove()
- do the push subscription and send back to the backend OR just call
sendSubscriptionToBackend
once removed
On unsubscribe/logout, we also need to check if we can remove sw push manager subscription (there are no more subscribed accounts): I'll rewrite the logic later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you come up with some issues to describe the outstanding tasks so we don't forget?
# Conflicts: # .eslintignore # app.vue # nuxt.config.ts # types/index.ts
I'm getting this error at the moment when I try to enable push notifications): TypeError: can't assign to property "daniel@roe.dev" on false: not an object (This is also happening with the default elk dev team mock.) I'm assuming an upstream change has caused this. |
Clear local storage: to support hide notification per account the type was changed from boolean to Record<string, boolean>, maybe we need to prepend also the server to the acct as the key. The entry to remove from local storage is called |
This PR includes:
dev-pwa
andbuild-pwa
respectivellyPWAPrompt.client.vue
andNotificationPreferences.client.vue
componentsen-US
andes-ES
localesapp.vue
anderror.vue
pagesprocess.env.VITE_DEV_PWA=true
: we dont want PWA on production yetPending to review:
Missing:
To test the pwa run
nr dev-pwa
ornr build-pwa && nr start
: on dev server you can debug the service worker and push notifications and clicks.Video running the dev server: https://streamable.com/b4cx5i
closes #101
closes #340
closes #84
supersedes #110