-
Notifications
You must be signed in to change notification settings - Fork 530
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
Add ability to opt in to Email communication #3212
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! However, when I try to access the stage, I'm denied permission. Is this supposed to happen?
Can you try to Sign in? |
Okay it works now! All good on my end |
@bcolsson As the author of the spec, do you approve these changes? Please check out the implementation on stage! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the specs, Contact email address
-> Contact email address (optional)
Receive the latest updates about localization at Mozilla, announcements about new Pontoon features, receive invitations to contributor events and more.
Good call on removing the duplicate "receive" here, but
Agree to Mozilla handling your personal information as explained in this Privacy Notice.
Missing a I
.
</div> | ||
</section> | ||
<ul class="check-list"> | ||
{{ Checkbox.checkbox('Email communication', class='field email-communications-enabled', attribute='email_communications_enabled', is_enabled=user.profile.email_communications_enabled, title='Receive email updates', help=settings.EMAIL_COMMUNICATION_HELP_TEXT) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should still be plural (communications)?
Are you suggesting we append (optional) to the label? All fields in the settings are optional. So I don't think it makes sense.
The first sentence is in the Imperative(?) mood. The second should be in the same. Hence - "Agree to ... your", rather than "I agree to ... my". |
That's how I interpret the specs, but let's wait for Bryan.
Good point, but it still sounds weird to me. Maybe add "By selecting/checking this, you"?
|
Updated the text. I used the same copy as in the standalone page: Accordingly, I also replaced "Receive" with "Get". |
These were both requests from legal. Let's leave "I agree.." as that's the wording the specifically requested. As for "Optional", legal was concerned that users would be confused in the instance where they want to have their marketing and login email be the same. In practice they just need to enable the check box to for the consent, but they requested to add something to clarify that specifying an email address in the |
@mathjazz Is there a way to add a line break to separate out the privacy notice to make it more distinct? |
OK, I see the confusion. How about we put the login email as the placeholder, so it would appear if contact email address is not set? How should we change the text underneath, which already implies the email address is optional ("If you would like to...")? Replace "please do so here" with "you can optionally do so here"?
Done. Please check on stage if that's better. I think it looks and reads even better with a double line break (you can use devtools to test that), but then it's not immediately clear the text is related to the checkbox. |
I think that's a great solution.
Agreed - I like the double line break. Also, I think the double line break is still understandable it relates to enabling email communications / the related checkbox due to indenting and the language "By enabling...". |
Stage updated with the placeholder and the additional line break: |
Staging looking good to me 👍 |
docker-compose.yml
Outdated
@@ -23,7 +23,7 @@ services: | |||
|
|||
# Database | |||
postgresql: | |||
image: postgres:15 | |||
image: postgres:13.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. Fixed!
Fix #3211.
Adds Email section to the user settings, which includes the existing Contact email address field and the newly added Email communication toggle. To match the existing UI patterns, I've used the shorter Checkbox label as required by the spec and joined the checkbox label and subtitle from the spec into the help text, which is configurable by instance.
The patch is deployed to stage:
https://mozilla-pontoon-staging.herokuapp.com/settings/
We'll also need to update the docs:
https://mozilla-l10n.github.io/localizer-documentation/tools/pontoon/users.html