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: pwa with push notifications #337

Merged
merged 87 commits into from
Dec 17, 2022
Merged

feat: pwa with push notifications #337

merged 87 commits into from
Dec 17, 2022

Conversation

userquin
Copy link
Member

@userquin userquin commented Dec 4, 2022

This PR includes:

  • pwa with prompt for update
  • pwa icons for production and dev/preview modes
  • pwa only on dev and local build scripts: dev-pwa and build-pwa respectivelly
  • service worker with push notifications and notification click capabilities added: we need to check switching between multiple accounts
  • PWAPrompt.client.vue and NotificationPreferences.client.vue components
  • notification settings button and notification preferences on notifications page
  • i18n properties for pwa and notification preferences: for en-US and es-ES locales
  • pwa configuration to modules folder (like i18n configuration)
  • pwa prompt on app.vue and error.vue pages
  • current push subscription is now requested on login: added entry to UserLogin
  • notifications settings and notification permission only shown with process.env.VITE_DEV_PWA=true: we dont want PWA on production yet

Pending to review:

  • check push subscription on multiple account switching
  • request notification permission logic

Missing:

  • settings options
  • allow show settings with pending notification permission: right now the settings button is disabled when desktop notifications shown

To test the pwa run nr dev-pwa or nr 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

@stackblitz
Copy link

stackblitz bot commented Dec 4, 2022

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@netlify
Copy link

netlify bot commented Dec 4, 2022

Deploy Preview for elk-zone ready!

Name Link
🔨 Latest commit 8e93f47
🔍 Latest deploy log https://app.netlify.com/sites/elk-zone/deploys/639e503ee0433700098ceb25
😎 Deploy Preview https://deploy-preview-337--elk-zone.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@userquin userquin requested a review from danielroe December 4, 2022 22:03
@userquin userquin mentioned this pull request Dec 4, 2022
@Shinigami92 Shinigami92 added the c: feature Request for new feature label Dec 5, 2022
@userquin userquin marked this pull request as ready for review December 5, 2022 12:49
isSupported,
notificationPermission,
subscribe,
unsubscribe,
Copy link
Member

Choose a reason for hiding this comment

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

unused?

Copy link
Member Author

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...

Copy link
Member Author

@userquin userquin Dec 13, 2022

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
@danielroe
Copy link
Member

Some UI thoughts:

  1. This should have a primary-coloured button with 'Save' or 'Close' something below it otherwise the most obvious thing to click is actually to disable notifications

CleanShot 2022-12-14 at 22 16 23@2x

  1. On FF, after enabling notifications the 'Enable notifications' CTA didn't disappear until I refreshed the page. But notifications don't seem to be working there. (Possibly a FF issue?)

Reworked the pr to hopefully remove the runtime/sw.ts workaround with nitro, but entirely possible I've missed some key step, so please test and let me know if there's anything I missed.

@danielroe
Copy link
Member

(FF was my error - it's working fine.)

@userquin
Copy link
Member Author

@danielroe

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.

@userquin
Copy link
Member Author

(FF was my error - it's working fine.)

Thx ❤️

@userquin userquin requested a review from danielroe December 15, 2022 20:36
@userquin
Copy link
Member Author

userquin commented Dec 15, 2022

@danielroe latest changes:

  • added push notifications policy to settings: masto push notifications seems to omit the policy on the update response!
  • added form with save and undo buttons: both buttons disabled when there are no changes
  • added form for disable push notifications button
  • added CommonRadio (type=radio)
  • added hover styles and focus styles to CommonRadio and CommonCheckbox
  • added animation for susbcribe, disable and save changes

}
else {
// Something went wrong, try to subscribe again
return unsubscribe({ registration, subscription }).then(
Copy link
Member Author

@userquin userquin Dec 16, 2022

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

Copy link
Member

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?

@danielroe
Copy link
Member

danielroe commented Dec 17, 2022

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.

@userquin
Copy link
Member Author

userquin commented Dec 17, 2022

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 elk-notification.

@danielroe danielroe merged commit f0c91a3 into main Dec 17, 2022
@danielroe danielroe deleted the userquin/feat-pwa branch December 17, 2022 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

add push notifications add PWA and service worker stuff Notifications w/ Web Push API
4 participants