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

Changed text and styling for the settings page #9240

Merged
merged 3 commits into from
Mar 3, 2021

Conversation

Manasa2850
Copy link
Member

@Manasa2850 Manasa2850 commented Feb 26, 2021

Fixes #6871 (<=== Add issue number here)

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

  • PR is descriptively titled 📑 and links the original issue above 🔗
  • 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 📁
  • screenshots/GIFs are attached 📎 in case of UI updation
  • 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

Thanks!

@Manasa2850 Manasa2850 requested a review from a team as a code owner February 26, 2021 20:21
@gitpod-io
Copy link

gitpod-io bot commented Feb 26, 2021

@Manasa2850
Copy link
Member Author

Before
before

After (for user login)
after_user

After (for moderator/admin login)
after_moderator

@codecov
Copy link

codecov bot commented Feb 26, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@9b874a9). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #9240   +/-   ##
=======================================
  Coverage        ?   82.24%           
=======================================
  Files           ?       98           
  Lines           ?     5852           
  Branches        ?        0           
=======================================
  Hits            ?     4813           
  Misses          ?     1039           
  Partials        ?        0           

Copy link
Contributor

@RuthNjeri RuthNjeri left a comment

Choose a reason for hiding this comment

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

Thanks @Manasa2850, this looks good so far, I've noticed this part is missing

Whenever there are new posts on topics I follow in the I want to receive an email section

Should it be present?


<div style="display: inline-flex; justify-content: space-between; width: 90%;">
<span>Do you want to receive a customized digest weekly?</span>
<span>For a weekly digest </span>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the issue says In a weekly digest...not sure if it is a strict change...I think it could remain as it is now...

This is the same for line 84

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we'd prefer the exact wording:

I want to receive an email

- Whenever there are new posts on topics I follow
- In a daily digest
- In a weekly digest

Can you make these changes? Thank you!!!!

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure I'll make these changes. 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.

@jywarren @RuthNjeri there was nothing corresponding to "Whenever there are new posts on topics I follow" earlier. I can take it up as a separate issue and add it in another PR if that's fine!

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @Manasa2850 I think that it is meant to be added as part of the notifications under I want to receive an email here https://github.com/publiclab/plots2/pull/9240/files#diff-60dea8a0195e299c872e9b749c642d0606caf5119e88bdfaab11fe378c46534dR67

I think there would be an entirely new notification switch though it's a great idea to follow that up in another issue to find out is there is a digest:topics for topics

Copy link
Member

Choose a reason for hiding this comment

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

Aha, ummm I think what is happening here is that these three options are actually a toggle. We can have only one of the three states, because a digest is /instead of/ updates for every post. Does that make sense? Is that distinction something we can spin out into a new issue or add to the original? Right now, i think we can skip the first option, but eventually it'll be like "do i want digests OR emails for each post?"

Does that make sense? Thanks and sorry for the confusion!

Copy link
Member

Choose a reason for hiding this comment

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

Or perhaps we can explicitly say that in help text above these toggles? Something like:

Email notifications are sent for each post in topics you follow. If you prefer to receive a digest, choose one of these options:

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure @jywarren. For now I'll just add a help text. I'll create another issue for creating a toggle for the options and then work on that.
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.

@RuthNjeri there is no digest:topics for topics as of now. I've created an issue #9260 for that and will try solving it.

@RuthNjeri
Copy link
Contributor

@Manasa2850 After this PR is merged, would you be interested in creating FTOs for the translations of the hardcoded texts within this file?

@Manasa2850
Copy link
Member Author

Yes @RuthNjeri I'd love to create FTOs for the translations!

@RuthNjeri
Copy link
Contributor

Thanks! I know @cesswairimu would love some for the SOC and Outreachy program!
#9229

@cesswairimu
Copy link
Collaborator

I sure would love that...thanks you both 🎉

@@ -47,7 +49,7 @@

<% if logged_in_as(['admin', 'moderator']) %>
<div style="display: inline-flex; justify-content: space-between; width: 90%;">
<span>Do you want to be notified by a email for moderation emails? </span>
<span>Moderation emails </span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this part of the changes in the issue? Also, line 149 and 163

Comment on lines +97 to +128
<% if current_user.can_moderate? %>
<div style="display: inline-flex; justify-content: space-between; width: 90%;">
<span>In a weekly digest for unmoderated posts</span>
<span>
<label style=" vertical-align: middle;" class="switch">
<p> Notification switch </p>
<input type="checkbox" name="digest:weekly:spam" <% if UserTag.exists?(current_user.id, 'digest:weekly:spam') %>checked<% end %>>
<span class="slider round"></span>
</label>
</span>
</div>

<br />
<br />

<div style="display: inline-flex; justify-content: space-between; width: 90%;">
<span>In a daily digest for unmoderated posts</span>
<span>
<label style=" vertical-align: middle;" class="switch">
<p> Notification switch </p>
<input type="checkbox" name="digest:daily:spam" <% if UserTag.exists?(current_user.id, 'digest:daily:spam') %>checked<% end %>>
<span class="slider round"></span>
</label>
</span>
</div>

<br />
<br />
<% end %>

<br />

Copy link
Contributor

Choose a reason for hiding this comment

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

Some of these lines differ from what was there previously, is this also part of the issue changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

@RuthNjeri the changes mentioned in the issue are just for when we login as a "user". I've extended it for "moderator" and "admin" logins too to maintain uniformity. Hence those changes.
Thanks!

@codeclimate
Copy link

codeclimate bot commented Mar 3, 2021

Code Climate has analyzed commit d74e22a and detected 0 issues on this pull request.

View more on Code Climate.

@Manasa2850 Manasa2850 closed this Mar 3, 2021
@Manasa2850 Manasa2850 reopened this Mar 3, 2021
@gitpod-io
Copy link

gitpod-io bot commented Mar 3, 2021

@Manasa2850 Manasa2850 closed this Mar 3, 2021
@Manasa2850 Manasa2850 reopened this Mar 3, 2021
@gitpod-io
Copy link

gitpod-io bot commented Mar 3, 2021

@Manasa2850 Manasa2850 closed this Mar 3, 2021
@Manasa2850 Manasa2850 reopened this Mar 3, 2021
@gitpod-io
Copy link

gitpod-io bot commented Mar 3, 2021

@Manasa2850
Copy link
Member Author

@jywarren @RuthNjeri could you please review it now? I think it can be merged now. I'll create the other 2 issues and start working on them after this is done!
Also, @RuthNjeri could you please help me with creating the translation FTOs. I'm not sure which files the changes need to be made in.
Thanks a lot for all the reviews done so far! 😃



<br />
<h4><b>Email notifications are sent for each post in topics you follow. If you prefer to receive a digest, choose one of these options: </b></h4>
Copy link
Member

Choose a reason for hiding this comment

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

Love it. Thank you so much!!!

@jywarren jywarren merged commit 19cd592 into publiclab:main Mar 3, 2021
@jywarren
Copy link
Member

jywarren commented Mar 3, 2021

Thanks so much for the careful work here!!! 🎉 🎉 🎉 🎉 🎉

reginaalyssa pushed a commit to reginaalyssa/plots2 that referenced this pull request Oct 16, 2021
* changes text for settings page

* added exact wording

* changed help text
billymoroney1 pushed a commit to billymoroney1/plots2 that referenced this pull request Dec 28, 2021
* changes text for settings page

* added exact wording

* changed help text
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.

Change texts for "settings" page
4 participants