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

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

Closed
jkasten2 opened this issue Apr 8, 2021 · 5 comments
Closed

Comments

@jkasten2
Copy link
Member

jkasten2 commented Apr 8, 2021

Scope

Creating this issue to track and discuss how to make AMP web push compatible with Automattic/newspack-plugin and OneSignal-WordPress-Plugin.

Status

<amp-web-push> Tag Details

Thanks to @westonruter for these details
Example of the amp-web-push component, configured with a custom element like so:

<amp-web-push
  layout="nodisplay"
  helper-iframe-url="https://example.com/helper-iframe.html"
  permission-dialog-url="https://example.com/permission-dialog.html"
  service-worker-url="https://example.com/service-worker.js"
></amp-web-push>

And then there are two custom elements for subscribing and unsubscribing to the notifications:

<!-- A subscription widget -->
<amp-web-push-widget
  visibility="unsubscribed"
  layout="fixed"
  width="250"
  height="80"
>
  <button on="tap:amp-web-push.subscribe">Subscribe to Notifications</button>
</amp-web-push-widget>

<!-- An unsubscription widget -->
<amp-web-push-widget
  visibility="subscribed"
  layout="fixed"
  width="250"
  height="80"
>
  <button on="tap:amp-web-push.unsubscribe">
    Unsubscribe from Notifications
  </button>
</amp-web-push-widget>

Integration TODO:

Thanks again to @westonruter for these details
So for configuration with OneSignal, I think it's mainly a matter of just supplying the relevant URLs to <amp-web-push>. See an example in the Newspack plugin's yet-unmerged PR Automattic/newspack-plugin#417 which seeks to add AMP-compatible push notifications. The amp-web-push attributes could be set accordingly:

So when amp_is_request() returns true, then that component could be rendered on the page. If false (or the function is not defined), then the plugin could continue doing what it does at present in OneSignal_Public::onesignal_header().

See also #258.

@adekbadek
Copy link
Contributor

Hi @jkasten2 👋
At Newspack, we're very keep to integrate OneSignal, and handling AMP in this plugin (instead of our workaround) seems like the best way forward. Is this planned to be implemented?

@rgomezp
Copy link
Contributor

rgomezp commented Mar 10, 2022

Howdy y'all,
It's been a while since we revisited this issue thread so I just wanted to check-in.

The plugin now automatically supports AMP (see PR). From the Newspack team, have you tested the latest plugin version? I believe it should work automatically but please let us know if there is other work needed to fully support the integration.

@adekbadek

@claudiulodro
Copy link

Thanks for checking and for adding compatibility! We have a number of people using the latest version with AMP-compatibility, and it's been working well for them so far.

If there is one feature request we can make it would be to have the AMP version respect the left/right settings from the WP Admin options screen. Currently it's hard-coded to the right corner. Totally not a big deal or hurry though. Feel free to close this issue. :)

@rgomezp
Copy link
Contributor

rgomezp commented Mar 10, 2022

Certainly! Great to hear it's working well.

I'll create a separate issue for the placement improvement.

@rgomezp rgomezp closed this as completed Mar 10, 2022
@gamebits
Copy link

Thanks for opening that issue regarding bell placement! #291

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

No branches or pull requests

5 participants