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.

Sign up for GitHub

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

Jump to bottom

feat(amp-plus): allow OneSignal script #1076

Merged
merged 5 commits into from
Nov 28, 2022
Merged

feat(amp-plus): allow OneSignal script #1076

merged 5 commits into from
Nov 28, 2022

Conversation

adekbadek
Copy link
Member

@adekbadek adekbadek commented Aug 3, 2021 •
edited
Loading

All Submissions:

Changes proposed in this Pull Request:

Alternate approach from #417 to handling push notifications. Instead of making them work within AMP constraints, using AMP Plus mode.

How to test the changes in this Pull Request:

  1. Deactivate the PWA plugin, if active (see Conflict of PWA and OneSignal (caused by Service Workers) #2104)
  2. Register and create a new app at app.onesignal.com – be sure to choose "WordPress" as the integration in the setup wizard
  3. Install OneSignal WP plugin (onesignal-free-web-push-notifications) and configure it
  4. Ensure AMP plugin is active and AMP Plus is enabled (NEWSPACK_AMP_PLUS_ENABLED environment variable is set to true)
  5. Visit the homepage in a new (non-incognito!) session
  6. Observe a notification bell icon appear in the lower right corner
  7. Click it to subscribe to notifications
  8. From the OneSignal dashboard, observe a new user has been added (in Audience -> Users)
  9. Send a new notification, observe that it works

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@adekbadek adekbadek mentioned this pull request Aug 17, 2021
Closed
6 tasks
@yogeshbeniwal
Copy link
Contributor

@adekbadek This will a good feature to have if its still a priority for development.

@adekbadek
Copy link
Member Author

We're discussing this internally – we'd like to implement this, but first establish a protocol for assessing performance impact of these added scripts.

@yogeshbeniwal
Copy link
Contributor

yogeshbeniwal commented Dec 7, 2021 •
edited
Loading

PageSpeed Insights is a good way to track performance impact for all enhancement.

@yogeshbeniwal yogeshbeniwal mentioned this pull request Feb 13, 2022
Open
@jkasten2 jkasten2 mentioned this pull request Mar 10, 2022
Closed
@adekbadek adekbadek force-pushed the feat/amp-plus-onesignal branch from 942d47b to 87fd06e Compare August 16, 2022 10:17
@adekbadek adekbadek marked this pull request as ready for review August 24, 2022 15:03
@adekbadek adekbadek requested a review from a team as a code owner August 24, 2022 15:03
@adekbadek adekbadek added [Status] Needs Review The issue or pull request needs to be reviewed and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Aug 24, 2022
@adekbadek adekbadek marked this pull request as draft August 24, 2022 15:09
@adekbadek adekbadek force-pushed the feat/amp-plus-onesignal branch from 87fd06e to 2869ef1 Compare August 24, 2022 15:49
@adekbadek
Copy link
Member Author

Blocked by OneSignal/OneSignal-WordPress-Plugin#298

@rgomezp
Copy link

rgomezp commented Oct 4, 2022

Howdy @adekbadek ,
298 is now merged and released.

Cheers

@adekbadek adekbadek marked this pull request as ready for review November 2, 2022 14:21
@adekbadek adekbadek added the [Status] Needs Review The issue or pull request needs to be reviewed label Nov 2, 2022
@leogermani
Copy link
Contributor

I tried to test it and went as far as seeing the Bell on the page.

When I click on it, a new blank browser windows shows up and closes very quickly, but nothing happens.

I got the same behavior both on desktop and mobile

Maybe I'm missing some configuration step?

@adekbadek
Copy link
Member Author

Might be a setup error, make sure you have AMP Plus enabled and the latest version of the OneSignal plugin.

I was able to set this up on a fresh site, but run into other issue: #2104. To test this properly, PWA plugin has to be deactivated. I've updated the description.

leogermani
leogermani approved these changes Nov 24, 2022
View reviewed changes
Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

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

Tested again disabling PWA and it worked fine.

@github-actions github-actions bot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Nov 24, 2022
@adekbadek adekbadek merged commit 790439a into master Nov 28, 2022
@adekbadek adekbadek deleted the feat/amp-plus-onesignal branch November 28, 2022 07:49
matticbot pushed a commit that referenced this pull request Dec 1, 2022
@semantic-release-bot
chore(release): 1.98.0-alpha.1 [skip ci]
e6ce0d4 
# [1.98.0-alpha.1](v1.97.1...v1.98.0-alpha.1) (2022-12-01)

### Bug Fixes

* **analytics:** improve the condition for using Site Kit's Analytics module ([5bc1c36](5bc1c36))
* **auth:** prevent autocomplete for honeypot field ([#2145](#2145)) ([9225726](9225726))
* harden subs meta box logic ([#2146](#2146)) ([35ea9fe](35ea9fe))
* **my-account:** hide account deletion for non-reader users ([85b2267](85b2267))
* **stripe:** wizard interaction issues ([#2142](#2142)) ([87918a7](87918a7))
* test emails ([11ea35a](11ea35a))

### Features

* add support to local lists in the newsletters wizard ([#2153](#2153)) ([9f81194](9f81194))
* add twitter pixel field ([#2144](#2144)) ([ee30061](ee30061))
* **amp-plus:** allow OneSignal ([#1076](#1076)) ([790439a](790439a))
* **analytics:** allow adding custom dimensions' values via filter ([1e20268](1e20268))
* disable jetpack's google analytics ([#2129](#2129)) ([1435b2a](1435b2a))
* perfmatters plugin defaults (with a special URL param) ([#2156](#2156)) ([da00a9b](da00a9b))
* **setup-wizard:** disambiguate starter vs. demo content ([ef125ca](ef125ca)), closes [#1701](#1701)
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.98.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Dec 12, 2022
@semantic-release-bot
chore(release): 1.98.0 [skip ci]
fce9d3e 
# [1.98.0](v1.97.3...v1.98.0) (2022-12-12)

### Bug Fixes

* **analytics:** improve the condition for using Site Kit's Analytics module ([5bc1c36](5bc1c36))
* **auth:** prevent autocomplete for honeypot field ([#2145](#2145)) ([9225726](9225726))
* harden subs meta box logic ([#2146](#2146)) ([35ea9fe](35ea9fe))
* improved existing Woo user subs handling ([#2162](#2162)) ([bdc25f5](bdc25f5))
* logic to update existing contact on subscription cancellation or deletion ([#2165](#2165)) ([68d7d30](68d7d30))
* **my-account:** hide account deletion for non-reader users ([85b2267](85b2267))
* **stripe:** wizard interaction issues ([#2142](#2142)) ([87918a7](87918a7))
* test emails ([11ea35a](11ea35a))

### Features

* add support to local lists in the newsletters wizard ([#2153](#2153)) ([9f81194](9f81194))
* add twitter pixel field ([#2144](#2144)) ([ee30061](ee30061))
* **amp-plus:** allow OneSignal ([#1076](#1076)) ([790439a](790439a))
* **analytics:** allow adding custom dimensions' values via filter ([1e20268](1e20268))
* disable jetpack's google analytics ([#2129](#2129)) ([1435b2a](1435b2a))
* perfmatters plugin defaults (with a special URL param) ([#2156](#2156)) ([da00a9b](da00a9b))
* **setup-wizard:** disambiguate starter vs. demo content ([ef125ca](ef125ca)), closes [#1701](#1701)
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.98.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

adekbadek added a commit that referenced this pull request Jan 6, 2023
@adekbadek
Revert "feat(amp-plus): allow OneSignal (#1076)"
9b3a191 
790439a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leogermani leogermani leogermani approved these changes

Assignees
No one assigned
Labels
released on @alpha released [Status] Approved The pull request has been reviewed and is ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@adekbadek @yogeshbeniwal @rgomezp @leogermani @matticbot