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

Feature/push notifications #2

Open
wants to merge 6 commits into
base: development
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented May 4, 2018

Usage

  • To test in browser simple load the app as usual, going to the Push Notification Page http://localhost:8000/push
  • You will need to hit the 'Subscribe' button first before you can use the test form.
  • To reset Notification preferences right-click the info button in chrome, and in the Notification dropdown select 'Ask (Default)'

Testing on Mobile

First you will need to install Local Tunnel ( https://localtunnel.github.io/www/ )
Basically run npm install -g localtunnel to install
Then run lt --port 8000 and you will be given a URL
Type the URL ( with '/push') into the browser, including 'https://' as Local Tunnel doesnt forward to https, and it is required for Service Workers to work
You might have issues on iOS. This all works on android, however I couldnt seem to get it to show up on iOS. Im not 100% sure if this is a localtunnel issue or something to do with the app / an error.
I think if we ran it in X-Code Simulator we should be able to see if theres an error.

Notes

The majority of this commit / investigative work is based on the Google Codelabs article
https://developers.google.com/web/fundamentals/codelabs/push-notifications/

Initially the code was based upon being able to send Push Notifications using the following tool:
https://web-push-codelab.glitch.me/
However I found it quite flakey so removed most of it. It can however still be used by going to the site and copying the public key it gives you, and adding it to the serviceWorker.js file.
Then after reloading the application, in the console copy the code beginning with {"endpoint": and paste it into the tool.

As a result of this, the updateSubscriptionOnServer() function in serviceWorker.js has been left in.

General Changes

In order to keep things neater and not flood index.js with code I've moved the Service Worker into its own folder in 'services', I wasnt sure if this is the right folder for it.
In that file Ive set it to export the registerWorker and subscribeUser functions, as well as the swRegistration variable itself so that they can be used in index.js / any components.

Also an eventListener for 'push' has been added to the sw.js file

Ive also had a few issues with caching so have added a variable enableCache to sw.js, which just stops the caching functions running when set to false

Ive made a few changes to webpack in how it was constructing the manifest.json file in order to get 'Save to Home Screen' working. Also removed publicPath as it was causing an issue with the image loading for the manifest.

Push Notification Example

I've created a React Component to be able to test Push Notifications called PushNotificationExample. Effectively it just imports the swRegistration variable it into the component and call swRegistration.showNotification(title, options)
Where the title and options (message) are then typed into the form by the user and then when the button is pressed a Push Notification is sent.
I also added the MMT Logo to the Push Notification

Push Subscribe Example

Initially I moved the Allow / Block notifications function to run in the Service Worker Registration request so it shows on page load the first time a user visits, however when testing on mobile it sometimes required a few page refreshes in order to work.
To allow for easier demonstrations / testing I decided to create a React component with a button instead that when clicked will set off the promt to allow a user to sign up for notifications.

Issues

There also seem to be a couple of bugs I can't seem to figure out;

  • When refreshing the app normally it comes back with a cached version and sometimes needs to be a hard refresh and reload.

james-mmt added 2 commits May 4, 2018 09:50
…moves around Service Worker registration code
…ebpack manifesto changes, Adds Add to home screen capability

componentDidMount () {
navigator.serviceWorker.ready.then((swRegistration) => {
console.log("hello world");

Choose a reason for hiding this comment

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

Can we change this to something more meaningful, or remove altogether?

})
}

buttonClicked = (event) => {

Choose a reason for hiding this comment

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

No need to pass the event object in here - a simple () will be fine

}

render () {
let isSubscribed = this.state.userSubscribed

Choose a reason for hiding this comment

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

I could be wrong; but this variable looks unnecessary

render () {
let isSubscribed = this.state.userSubscribed

const button = this.state.userSubscribed ? (

Choose a reason for hiding this comment

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

We don't create components like this. It would be better to make this elsewhere and import it

title: '',
message: ''
}
state = this._initState

Choose a reason for hiding this comment

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

Assigning state like this doesn't quite seem right. You could just change _initState to state

state = this._initState

handleChange = (event) => {
const { value, name } = event.target

Choose a reason for hiding this comment

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

Here's a perfect time to use argument desctructuring 😉 You can grab these in the function ()

So it'll be: handleChange = ({ value, name }) => {}

badge: '../../assets/mmt.png'
}
swRegistration.showNotification(title, options)
this.setState(this._initState)

Choose a reason for hiding this comment

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

Might be worth having a function called resetState or similar that will setState to your initial set up

onChange={this.handleChange}
styleName='message'
/>
<button styleName='button' type="submit">Send Notification</button>

Choose a reason for hiding this comment

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

Here's where you could import the button component you made earlier 😄

return outputArray;
}

function subscribeUser() {

Choose a reason for hiding this comment

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

Can we change these to ES6 functions please?

src/sw.js Outdated
'/assets/bundle.js',
'/assets/styles.css'
])
if(enableCache) {

Choose a reason for hiding this comment

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

space between if and ()

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

Successfully merging this pull request may close these issues.

2 participants