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

Added Service worker for notification on mobile phones #6158

Merged
merged 1 commit into from
Aug 19, 2019

Conversation

namangupta01
Copy link
Member

@namangupta01 namangupta01 commented Aug 19, 2019

Closes #6020

@namangupta01
Copy link
Member Author

@jywarren I guess this would work. Unable to test on mobile because on mobile it only works with https.

@namangupta01
Copy link
Member Author

But Working on desktop.

@namangupta01
Copy link
Member Author

Pushed to unstable but not able to test on unstable because of error while creating a comment. So can we merge this so that I can test for mobile on stable? Working on the desktop is tested.
Thanks!

@jywarren
Copy link
Member

Sure, but we don't have https on stable either. Is there a way to test locally on https using a passenger setting?

@namangupta01
Copy link
Member Author

I guess we have

@namangupta01
Copy link
Member Author

Screen Shot 2019-08-20 at 2 24 38 AM

@namangupta01
Copy link
Member Author

I tried with thin using force SSL but that was causing WebSocket upgrade error.

@jywarren
Copy link
Member

jywarren commented Aug 19, 2019 via email

@jywarren
Copy link
Member

OK! I'll merge this but please be careful to file any fixes swiftly before we publish to production in the next day or two! Thanks, Naman!

@jywarren jywarren merged commit c24c707 into master Aug 19, 2019
@namangupta01
Copy link
Member Author

Sure! 👍

@namangupta01
Copy link
Member Author

Let's check on stable. Hopefully, it will work.

@namangupta01
Copy link
Member Author

Yayyy! It worked! 🎉 🎉

@namangupta01
Copy link
Member Author

Screen Shot 2019-08-20 at 2 49 24 AM

@namangupta01
Copy link
Member Author

I got two duplicate notifications on the phone. Here I have fixed it #6159.
Thanks!

@jywarren
Copy link
Member

jywarren commented Aug 19, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Browser Notification API work on phone
2 participants