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

Configurable Email Settings #2985

Merged
merged 16 commits into from
Jul 20, 2018
Merged

Conversation

grvsachdeva
Copy link
Member

@grvsachdeva grvsachdeva commented Jul 5, 2018

Fixes #213

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • PR body includes fixes #0000-style reference to original issue #
  • ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays.

Thanks!

@ghost ghost assigned grvsachdeva Jul 5, 2018
@ghost ghost added the in progress label Jul 5, 2018
@plotsbot
Copy link
Collaborator

plotsbot commented Jul 5, 2018

1 Warning
⚠️ It looks like you merged from master in this pull request. Please rebase to get rid of the merge commits – you may want to rewind the master branch and rebase instead of merging in from master, which can cause problems when accepting new code!
1 Message
📖 @Gauravano Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.

Generated by 🚫 Danger

@jywarren
Copy link
Member

jywarren commented Jul 5, 2018

Oooh, exciting!!! 👍

@grvsachdeva grvsachdeva added summer-of-code feature explains that the issue is to add a new feature labels Jul 12, 2018
@ViditChitkara
Copy link
Member

Hi @Gauravano, I guess you can now use the customize_digest function in this. It is present in user.rb

@grvsachdeva
Copy link
Member Author

Ok, @ViditChitkara, I will add it. Thanks!

@grvsachdeva
Copy link
Member Author

Hi @ViditChitkara, I have implemented digest settings in this PR as we have discussed, you can try at https://unstable.publiclab.org/ , I have pushed there. Let me know if any change is required. Thanks!

@grvsachdeva grvsachdeva requested a review from jywarren July 18, 2018 14:44
@grvsachdeva
Copy link
Member Author

Hi @jywarren @ViditChitkara this PR is complete. I have incorporated 1 setting and 2 digest settings in this. Please review it, thanks!

@jywarren
Copy link
Member

This is very exciting - can you post a screenshot once more so we can do a final look at it?

@grvsachdeva
Copy link
Member Author

Here's default behavior when the setting is turned on: http://gauravsachdeva.me/default_behaviour.gif

Here's the GIF when setting is turned off: http://gauravsachdeva.me/no_comment.gif

@grvsachdeva
Copy link
Member Author

@jywarren
Here's demo for Digest Settings: http://gauravsachdeva.me/digest_settings.gif
I have been asked by Vidit to show two switches on settings page one for daily and other for weekly. The user should be able to choose only one at a time 'daily or weekly' and can turn both off. I guess the PR serves that.

@jywarren
Copy link
Member

This is super. 🎉 Do you want to think with @ViditChitkara about how to ensure /settings and /subscriptions don't confuse people? Could they be on the same page, or could /settings be shown in the menu as Digest settings? Also maybe a link from /subscriptions to /settings ? I just imagine people wondering "what's the difference between /settings and /subscriptions"?

Then we should be good to go on this.

@grvsachdeva
Copy link
Member Author

We would be adding more settings in the future, currently, 3 more settings are on my checklist too. So, it would be better to keep it as a different page. Coming to the name of the page - as of now all settings will be related to Email, Notification Settings or Email Settings looks good to me. And, yes, adding a link in subscriptions to settings page is a good idea. What do you think @ViditChitkara @jywarren?

@jywarren
Copy link
Member

jywarren commented Jul 18, 2018 via email

@ViditChitkara
Copy link
Member

"Email settings" looks good to me!!

@grvsachdeva
Copy link
Member Author

Changing the name and adding a link then. Thanks!

@grvsachdeva
Copy link
Member Author

Hi @jywarren here's addition in Subscriptions page
design_1

Please suggest improvements. Thanks!

@jywarren
Copy link
Member

jywarren commented Jul 20, 2018 via email

@grvsachdeva
Copy link
Member Author

Done!

done_

@jywarren
Copy link
Member

Super, ready then? 😄

@grvsachdeva
Copy link
Member Author

yes!

@jywarren jywarren merged commit 6f353b2 into publiclab:master Jul 20, 2018
@ghost ghost removed the review-me label Jul 20, 2018
@jywarren
Copy link
Member

Hooray!!! 🎉

SrinandanPai pushed a commit to SrinandanPai/plots2 that referenced this pull request May 5, 2019
* comment setting

* settings page render

* settings page

* controller logic

* comment direct done

* activate button on change

* digest settings

* fix

* fix

* fix 2

* clean up

* clean up

* tests

* design change

* tweak
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature explains that the issue is to add a new feature summer-of-code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

configurable email notification preferences
4 participants