Push Notifications #417

jeffersonrabb · 2020-01-31T11:49:09Z

All Submissions:

Have you followed the Newspack Contributing guideline?
Does your code follow the WordPress' coding standards and VIP Go coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Implements basic support for AMP-compatible Push Notifications using One Signal. This PR still has some rough edges but is ready for review/discussion.

Video demo of subscribing: https://cloudup.com/cyyfKRs0tG7

How to test the changes in this Pull Request:

Navigate to Newspack Engagement Wizard, click on "Push" tab.
Click toggle to install One Signal plugin.
Go to https://onesignal.com/ create a free account, then create an App for "Web Push."
In Settings->Keys/IDs find your APP ID and REST API KEY. Copy paste these into the corresponding fields in the engagement wizard.
Edit a widget area. Add the Newspack Push Notifications widget to a an area.
Try testing in an AMP request. You should see a Subscribe button. Clicking the button opens a popup browser window, and in the window you will be prompted to subscribe.
After subscribing you should see the number of subscribers increment in the One Signal dashboard.
If you don't, you may be blocking notifications. On advice of @adekbadek I usually would create a new Chrome profile (click your Avatar in the upper right, choose ADD+), and subscribe with that user.
After successfully subscribing, try creating and publishing a post and you should receive a push notification.

Open Questions

For now it appears the pop-up window is required. I'll be opening an AMP issue seeking improvement but for now it seems we're stuck with it. How should we style to make it a more palatable experience? cc: @thomasguillot
How can we make the widget UI better?
Should we add a block as well? If so, it would presumably live in Newspack Blocks.
Should we guide users through acquiring and configuring their One Signal account? There is some information about doing this in the One Signal plugin — we could just send new users over there.
I believe the implementation will not currently work in non-AMP. This should be addressed.

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?

westonruter · 2020-02-03T21:39:41Z

includes/class-pwa.php

+				layout="nodisplay"
+				helper-iframe-url="<?php echo esc_url( $base ); ?>/helper-iframe.html?appId=<?php echo esc_attr( $one_signal_api_key ); ?>"
+				permission-dialog-url="<?php echo esc_url( $base ); ?>/permission-dialog.html?appId=<?php echo esc_attr( $one_signal_api_key ); ?>"
+				service-worker-url="<?php echo esc_url( $base ); ?>/service-worker.js?appId=<?php echo esc_attr( $one_signal_api_key ); ?>"


Suggested change

service-worker-url="<?php echo esc_url( $base ); ?>/service-worker.js?appId=<?php echo esc_attr( $one_signal_api_key ); ?>"

service-worker-url="<?php echo esc_url( $base ); ?>/service-worker.js?appId=<?php echo esc_attr( $one_signal_api_key ); ?>"

service-worker-scope="<?php echo esc_url( trailingslashit( $base ) ); ?>"

westonruter · 2020-02-04T21:26:55Z

I wonder if this PR should end up being merged if it works? Perhaps it should rather be upstreamed as part of OneSignal/OneSignal-WordPress-Plugin#187?

adekbadek

Works as advertised, there're just some nitpicky linting errors. Also, Weston's suggestion to add service-worker-scope seems reasonable.

thomasguillot

On this screen:

I've just clicked on the toggle button to enable the push notifications and I get a [object Object] in both inputs.
It needs some sort of help text to explain how to get the App ID and API key with a link to One Signal (or in the future to the documentation page of this feature on our website)

It would be nice to have the option to change the copy of both buttons.
Why do we display both buttons but use visibility: hidden to hide one of the two? Visually it creates a massive gap. If we have to "display" both, can we at least use display: none?
I'd definitely suggest a block to make this feature a bit more flexible within the content.

How should we style to make it a more palatable experience?

Quite frankly I'm not sure. This looks terrible 😂 Can we reduce the window size? Also if you click on the x to close the pop-up, the window doesn't close... it's just so bad.

When AMP is set to Transitional or Reader, we get this pop-up at the top of the screen. Looks like it's coming from One Signal. Can we style it so it doesn't look so out-of-place? Right now, all I can think is a fake spam advert with the big ugly icon.

jkasten2

Overall this PR looks great, just a few nits and questions! Thanks for taking the time to integrate OneSignal directly in this plugin.

includes/class-pwa.php

assets/wizards/engagement/views/push-notifications/index.js

jkasten2 · 2020-02-26T01:47:41Z

includes/class-pwa.php

+		$path = null;
+
+		if ( substr( $raw_uri, 0, strlen( '/helper-iframe.html' ) ) === '/helper-iframe.html' ) {
+			$path = dirname( NEWSPACK_PLUGIN_FILE ) . '/assets/raw/amp-web-push-helper-frame.html';


Interested to know why not use /assets/raw/amp-web-push-helper-frame.html directly with the <amp-web-push> tag? Instead of going through /helper-iframe.html then mapping it.

jkasten2 · 2020-02-26T01:50:13Z

includes/configuration_managers/class-onesignal-configuration-manager.php

+		if ( ! $this->is_configured() ) {
+			return $this->unconfigured_error();
+		}
+		$settings                     = \OneSignal::get_onesignal_settings();


Since you are using OneSignal::save_onesignal_settings() and OneSignal::get_onesignal_settings() we will make a note that these are public interfaces should not change in the OneSignal-WordPress-Plugin repo.

…ml and /permission-dialog.htmfeat: render required push notification html files: /helper-iframe.html and /permission-dialog.htmll

this was brought up in client validation, not in AMP-WP validator, for some reason

from https://amp.dev/documentation/examples/components/amp-web-push/sw.js

…otification_urls

…-web-push implementation.

…nected to onesignal plugin.

…tion subscribe UI.

…d by status of plugin install.

Move push notification html files from assets to includes directory. assets is no longer included in release archives.

jeffersonrabb · 2020-03-04T12:51:53Z

includes/class-pwa.php

@@ -16,6 +16,13 @@
 */
 class PWA {

+	/**


@rgomezp this commit adds the service worker scope you recommended.

@jeffersonrabb Looks good to me!

adekbadek · 2021-08-17T14:43:48Z

Closing for now in favour of #1076

jeffersonrabb force-pushed the add/push-notifications branch from 2442b78 to ca60a86 Compare February 1, 2020 19:48

westonruter reviewed Feb 3, 2020

View reviewed changes

jeffersonrabb force-pushed the add/push-notifications branch from ca60a86 to ba261de Compare February 5, 2020 16:14

westonruter mentioned this pull request Feb 14, 2020

[WIP] Prototype adding direct support for AMP and PWA OneSignal/OneSignal-WordPress-Plugin#187

Closed

jeffersonrabb force-pushed the add/push-notifications branch from b47c96b to 5dbdabe Compare February 20, 2020 05:45

jeffersonrabb requested review from adekbadek and thomasguillot February 20, 2020 06:04

jeffersonrabb self-assigned this Feb 20, 2020

jeffersonrabb marked this pull request as ready for review February 20, 2020 06:04

jeffersonrabb added [Status] Needs Review The issue or pull request needs to be reviewed [Status] Needs Design Review labels Feb 20, 2020

adekbadek approved these changes Feb 20, 2020

View reviewed changes

adekbadek 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 Feb 20, 2020

thomasguillot requested changes Feb 21, 2020

View reviewed changes

jkasten2 reviewed Feb 26, 2020

View reviewed changes

jeffersonrabb force-pushed the add/push-notifications branch from b781c32 to e604134 Compare March 2, 2020 21:30

jeffersonrabb and others added 12 commits March 3, 2020 23:55

feat: UI for switching push notifications on and off.

fc0322d

refactor: define push notifications constant in PWA class.

bb3f1ed

feat: render required push notification html files: /helper-iframe.ht…

0b4c007

…ml and /permission-dialog.htmfeat: render required push notification html files: /helper-iframe.html and /permission-dialog.htmll

feat: add amp push notification components (not yet working).

ee90cb8

fix: add id attr to amp-web-push

efd27b7

fix: add layout attr to amp-web-push

edefab6

this was brought up in client validation, not in AMP-WP validator, for some reason

feat: add Service Worker

299ec50

from https://amp.dev/documentation/examples/components/amp-web-push/sw.js

fix: check if path starts with instead of exact path in render_push_n…

54a9c77

…otification_urls

feat: set up with OneSignal

ad06e01

feat: One Signal api key. UI in engagement wizard, integration in amp…

b24ac24

…-web-push implementation.

feat: move the push notification subscribe ui to a widget.

18768cc

feat: add One Signal plugin to supported list.

174c5f3

jeffersonrabb added 8 commits March 3, 2020 23:55

feat: PluginToggle calls onReady property after updates to plugin data.

32bfe73

feat: use PluginToggle to control push notifications state.

6d3651e

feat: override PluginToggle name for push notifications.

52987b2

feat: onesignal configuration manager. engagement wizard settings con…

e37988e

…nected to onesignal plugin.

fix: missing configuration manager file.

7c2040f

fix: access One Signal plugin app id key when rendering push notifica…

cb4b94f

…tion subscribe UI.

feat: engagement->push screen - visibility of UPDATE button determine…

7eb6559

…d by status of plugin install.

fix: relocate push notification html files

397965e

Move push notification html files from assets to includes directory. assets is no longer included in release archives.

jeffersonrabb force-pushed the add/push-notifications branch from 7067221 to 397965e Compare March 4, 2020 05:55

jeffersonrabb added 3 commits March 4, 2020 00:03

refactor: better variable name for OneSignal app ID

e053b25

fix: no space in OneSignal

67c2823

fix: add service-worker-scope

4ef2275

jeffersonrabb commented Mar 4, 2020

View reviewed changes

jeffersonrabb added Work In Progress and removed [Status] Approved The pull request has been reviewed and is ready to merge labels Mar 5, 2020

westonruter mentioned this pull request Mar 19, 2020

amp-web-push: unnecessary pop-up window for push notification permission request for AMP pages on origin/SxG ampproject/amphtml#27303

Open

dkoo mentioned this pull request Jul 8, 2020

NTG events: user interaction #561

Closed

westonruter mentioned this pull request Apr 8, 2021

Investigate removing page controller requirement for the ServiceWorker OneSignal/OneSignal-Website-SDK#615

Closed

jkasten2 mentioned this pull request Apr 8, 2021

AMP web push with Automattic/newspack-plugin + OneSignal-WordPress-Plugin OneSignal/OneSignal-WordPress-Plugin#272

Closed

adekbadek mentioned this pull request Aug 3, 2021

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

Merged

6 tasks

adekbadek closed this Aug 17, 2021

thomasguillot deleted the add/push-notifications branch October 15, 2021 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Push Notifications #417

Push Notifications #417

jeffersonrabb commented Jan 31, 2020 •

edited

Loading

westonruter Feb 3, 2020

westonruter commented Feb 4, 2020 •

edited

Loading

adekbadek left a comment •

edited

Loading

thomasguillot left a comment

jkasten2 left a comment

jkasten2 Feb 26, 2020

jkasten2 Feb 26, 2020

jeffersonrabb Mar 4, 2020

jkasten2 Mar 11, 2020

adekbadek commented Aug 17, 2021

	service-worker-url="<?php echo esc_url( $base ); ?>/service-worker.js?appId=<?php echo esc_attr( $one_signal_api_key ); ?>"
	service-worker-url="<?php echo esc_url( $base ); ?>/service-worker.js?appId=<?php echo esc_attr( $one_signal_api_key ); ?>"
	service-worker-scope="<?php echo esc_url( trailingslashit( $base ) ); ?>"

Push Notifications #417

Push Notifications #417

Conversation

jeffersonrabb commented Jan 31, 2020 • edited Loading

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Open Questions

Other information:

westonruter Feb 3, 2020

Choose a reason for hiding this comment

westonruter commented Feb 4, 2020 • edited Loading

adekbadek left a comment • edited Loading

Choose a reason for hiding this comment

thomasguillot left a comment

Choose a reason for hiding this comment

jkasten2 left a comment

Choose a reason for hiding this comment

jkasten2 Feb 26, 2020

Choose a reason for hiding this comment

jkasten2 Feb 26, 2020

Choose a reason for hiding this comment

jeffersonrabb Mar 4, 2020

Choose a reason for hiding this comment

jkasten2 Mar 11, 2020

Choose a reason for hiding this comment

adekbadek commented Aug 17, 2021

jeffersonrabb commented Jan 31, 2020 •

edited

Loading

westonruter commented Feb 4, 2020 •

edited

Loading

adekbadek left a comment •

edited

Loading