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

Fixed notification permission prompt #6003

Merged
merged 3 commits into from
Jul 10, 2019
Merged

Fixed notification permission prompt #6003

merged 3 commits into from
Jul 10, 2019

Conversation

namangupta01
Copy link
Member

Fix for #5972.

@namangupta01 namangupta01 mentioned this pull request Jul 10, 2019
Merged
jywarren
jywarren reviewed Jul 10, 2019
View reviewed changes
app/assets/javascripts/application.js Outdated
@@ -56,3 +56,10 @@
//= require keybindings.js
//= require realtime_username_validation.js
//= require cable.js

Notification.requestPermission().then(function (permission) {
Copy link
Member

Choose a reason for hiding this comment

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

Hi! I guess I'd prefer this to be in a sub-file like notifications.js or could it fit in the cable.js file? Just because this is mostly a file for concatenating things. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! Let's merge and test this out.

@jywarren jywarren merged commit 75f3a7e into master Jul 10, 2019
@jywarren
Copy link
Member

🎉

@namangupta01
Copy link
Member Author

namangupta01 commented Jul 10, 2019 via email

@jywarren
Copy link
Member

Actually stable server is auto deployed from master branch now! So it should be live.

@namangupta01
Copy link
Member Author

namangupta01 commented Jul 11, 2019 via email

@jywarren
Copy link
Member

jywarren commented Jul 11, 2019 via email

@namangupta01
Copy link
Member Author

Any idea why commenting on unstable is causing error? Is it due to mailers?

@namangupta01
Copy link
Member Author

Screen Shot 2019-07-12 at 12 23 44 AM

@jywarren
Copy link
Member

jywarren commented Jul 11, 2019 via email

@namangupta01
Copy link
Member Author

I guess this is not related to manually creating comments? Isn't it related to Tweet's feature? What do you think?

@namangupta01
Copy link
Member Author

Yeah I got it! It is https://sentry.io/share/issue/0b9d839e6e86491cb4bc1db56023ad76/ SMTP related error!

@namangupta01
Copy link
Member Author

I commented the mailing notification code and testing now on unstable.

@jywarren
Copy link
Member

jywarren commented Jul 11, 2019 via email

@jywarren
Copy link
Member

Yay!!!

image

@jywarren
Copy link
Member

Although, it asks you for permission before you're logged in!

@jywarren
Copy link
Member

@namangupta01 would you see if you could trigger it only on the dashboard?

@jywarren
Copy link
Member

This worked!

image

But not on my phone, for some reason? Chrome Android...

@namangupta01
Copy link
Member Author

namangupta01 commented Jul 12, 2019 via email

@namangupta01
Copy link
Member Author

namangupta01 commented Jul 12, 2019 via email

enviro3 pushed a commit to enviro3/plots2 that referenced this pull request Aug 15, 2019
@namangupta01 @enviro3
Fixed notification permission prompt (publiclab#6003)
26d9649 
* Fixed notification permission prompt

* Minor Change

* Moved notification prompt code from application.js to cable.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jywarren jywarren jywarren left review comments

Assignees
No one assigned
Labels
gsoc summer-of-code
Projects
Notification System
Awaiting triage
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@namangupta01 @jywarren