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

Email Settings: add "valid email" and "verified email" status #7083

Merged
merged 8 commits into from
May 31, 2024

Conversation

shaunanoordin
Copy link
Member

@shaunanoordin shaunanoordin commented Apr 24, 2024

PR Overview

Staging branch URL: https://pr-7083.pfe-preview.zooniverse.org
Towards #7032

This PR adds "🟢 valid / 🔴 invalid email" and "🟢 verified / 🔴 unverified email" indicators to the Email Settings page (under User -> Settings -> Email)

  • These indicators are based on the user.valid_email and user.confirmed_at values of the User resource.
Screenshot 2024-04-24 at 02 55 55

Status

Ready for review.

Next steps: the "request confirmation email" functionality requires either

  1. a fairly custom (and complex) fetch() function inside email.jsx to POST to panoptes.zooniverse.org/api/users/confirmation with the auth token inside the code, or
  2. an update to PJC to do the same, or
  3. as a placeholder, a simple link to https://panoptes.zooniverse.org/users/confirmation/new

For this PR, the "request confirmation email" has been disabled. (I got halfway through option 1 but ran into some bugs due to a malformed request, and option 3 hit a small snag when I realised I need to check the env to get the correct URL.)

@shaunanoordin shaunanoordin requested a review from a team April 24, 2024 02:15
@coveralls
Copy link

coveralls commented Apr 24, 2024

Coverage Status

coverage: 56.994% (+0.01%) from 56.98%
when pulling 6125950 on email-verification-status
into b8cf174 on master.

Copy link
Contributor

@eatyourgreens eatyourgreens left a comment

Choose a reason for hiding this comment

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

Some comments on how to make this work for screen reader users.

app/pages/settings/email.jsx Show resolved Hide resolved
<div>
{(isEmailValid)
? <div>
<i className='fa fa-check-circle' style={{ color: '#51db72' }} />
Copy link
Contributor

Choose a reason for hiding this comment

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

This icon will be missed by a screen reader. As an aside, icon fonts aren’t really used in web development any more, because they can cause accessibility problems.

app/pages/settings/email.jsx Outdated Show resolved Hide resolved
app/pages/settings/email.jsx Show resolved Hide resolved
@shaunanoordin shaunanoordin changed the title Email Settings: Email Settings: add "valid email" and "verified email" status Apr 24, 2024
@shaunanoordin shaunanoordin force-pushed the email-verification-status branch from 21cfd06 to f65ba13 Compare May 23, 2024 15:29
Copy link
Contributor

@goplayoutside3 goplayoutside3 left a comment

Choose a reason for hiding this comment

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

Looks good! Please remove the new translations from all dictionaries other than en.js, and good to consider the accessibility comments from Jim.

On this PR branch I'm able to sign-in to my account and see that my email is valid and verified. If I create a new account, the email correctly shows it's unverified until I follow the confirm-email email. I can also edit the email validity of my account by removing the "@". I'll note that if my account has an invalid email, and I edit it to a valid email, a page refresh is not required for the settings indicator to change to "valid". However, if I change from a valid email to an invalid email, a page refresh is required to see the "invalid" indicator. Not a blocking behavior, but worth noting the quirk.

app/locales/ar.js Outdated Show resolved Hide resolved
@goplayoutside3 goplayoutside3 self-assigned this May 30, 2024
@shaunanoordin shaunanoordin force-pushed the email-verification-status branch from f65ba13 to e568565 Compare May 31, 2024 22:40
@shaunanoordin
Copy link
Member Author

Thank you! 👍 I'll address the following feedback before I merge:

  • accessibility.
  • removing non-'en.json' translations.

The "request confirmation email" functionality will appear in a follow up PR.

@shaunanoordin shaunanoordin force-pushed the email-verification-status branch from e568565 to 583c957 Compare May 31, 2024 22:48
@shaunanoordin shaunanoordin enabled auto-merge (squash) May 31, 2024 23:06
@shaunanoordin shaunanoordin merged commit 7f38b4c into master May 31, 2024
5 checks passed
@shaunanoordin shaunanoordin deleted the email-verification-status branch May 31, 2024 23:06
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.

4 participants