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

Enable UI Part for Browser Notification #6105

Merged
merged 3 commits into from
Aug 15, 2019
Merged

Conversation

namangupta01
Copy link
Member

Closes #6039

@namangupta01 namangupta01 changed the title Enabled UI Part for Browser Notification Enable UI Part for Browser Notification Aug 4, 2019
@namangupta01 namangupta01 reopened this Aug 4, 2019
@namangupta01
Copy link
Member Author

Restarted Build

@namangupta01
Copy link
Member Author

@jywarren lets gets this merge.

@jywarren
Copy link
Member

jywarren commented Aug 9, 2019

Looks awesome. Can you upload a gif, and would you mind adding this page to the auto-screenshot system tests so we can see that page too? Thank you!!!

@namangupta01
Copy link
Member Author

Yeah sure! Will do it.

@namangupta01
Copy link
Member Author

ezgif com-video-to-gif

@namangupta01
Copy link
Member Author

Adding this in system test screenshot as well.

@namangupta01
Copy link
Member Author

Added auto screenshot system test for the settings page.

@namangupta01
Copy link
Member Author

Restarting Build.

@namangupta01
Copy link
Member Author

Finally test passed. Can we merge this? @jywarren

@jywarren jywarren merged commit a31a09a into master Aug 15, 2019
@jywarren
Copy link
Member

Awesome!!! Great work!

@jywarren
Copy link
Member

This seems to be working for @warren notifications on stable! Just tested! However I got 2 duplicate notifications. Can we catch that?

image

Let's open a new issue to track that!

Also, I didn't seem to get Do you want to receive browser notification for all ? as I tried leaving a few comments at https://stable.publiclab.org/questions/mimiss/08-05-2019/testing-a-question but only the one with the @ actually triggered it.

@namangupta01
Copy link
Member Author

Yeah! it is currently working for mentioned username, I will add it to all the rest cases. It won't be much work since I only have to add just one line of code in every case. Will add it today. Thanks

@jywarren
Copy link
Member

jywarren commented Aug 15, 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.

Add Notification Enabling UI part
2 participants