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.

Sign up for GitHub

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

Jump to bottom

Block Promotional Emails UI #1545

Merged
merged 20 commits into from
Feb 25, 2022
Merged

Block Promotional Emails UI #1545

merged 20 commits into from
Feb 25, 2022

Conversation

maxxcrawford
Copy link
Contributor

@maxxcrawford maxxcrawford commented Feb 14, 2022 •
edited
Loading

Summary

Blocked until mozilla-l10n/fx-private-relay-l10n#61 and mozilla-l10n/fx-private-relay-l10n#63 are merged

This PR lets users set their email forwards prefs to "Block Promotional Emails". This PR depends on/is blocked by PR #1530

TODO:

Screenshot (if applicable)

image

image

image

How to test

Checklist

  • l10n dependencies have been merged, if any.
  • All acceptance criteria are met.
  • I've added or updated relevant docs in the docs/ directory.
  • All UI revisions follow the coding standards, and use Protocol tokens where applicable (see /static/scss/libs/protocol/css/includes/tokens/dist/index.scss).
  • Commits in this PR are minimal and have descriptive commit messages.

@maxxcrawford maxxcrawford changed the title Mpp 1347 critical emails UI Critical Emails UI Feb 14, 2022
@maxxcrawford maxxcrawford changed the base branch from main to start-critical-emails-feature-mpp-1347 February 14, 2022 21:58
@maxxcrawford maxxcrawford force-pushed the mpp-1347-critical-emails-ui branch from ae20613 to 773972c Compare February 14, 2022 22:00
@maxxcrawford maxxcrawford added the 🚧 WIP Work in Progress label Feb 15, 2022
@maxxcrawford maxxcrawford changed the base branch from start-critical-emails-feature-mpp-1347 to main February 15, 2022 19:12
@maxxcrawford maxxcrawford force-pushed the mpp-1347-critical-emails-ui branch 3 times, most recently from b0802fb to c2a4bd8 Compare February 18, 2022 19:32
@maxxcrawford maxxcrawford removed the 🚧 WIP Work in Progress label Feb 18, 2022
@maxxcrawford maxxcrawford marked this pull request as ready for review February 18, 2022 22:10
@maxxcrawford maxxcrawford requested a review from Vinnl February 22, 2022 15:31
@maxxcrawford maxxcrawford force-pushed the mpp-1347-critical-emails-ui branch from e6929c5 to dcccf6e Compare February 22, 2022 16:28
Vinnl
Vinnl reviewed Feb 23, 2022
View reviewed changes
Copy link
Collaborator

@Vinnl Vinnl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

A bunch of nits (i.e. non-blocking), and I encountered the following issues, the last of which I think could be blocking (but possibly has an easy fix - see my inline comments):

Accessibility

The new control is unreachable via keyboard. I think we can live with that for the short term, as long as we make sure to resolve that in the React version where react-aria can take care of much of that for us - but that's going to be the most complex part of the React implementation, so mentally prepare for that 😅

Toggle alignment

For free users, the toggle no longer is aligned with the label:

image

Inconsistent state

Hmm, I can't reproduce this consistently, but I've been able to get my aliases in an inconsistent state:

image

("None" is highlighted, but otherwise everything shows (correctly) that it is set to block promotional emails.)

This only happened to a single alias at first, but started happening to the ones I created afterwards as well, so looks like it's not a rare issue :/

privaterelay/templates/includes/alias-card.html Outdated Show resolved Hide resolved
privaterelay/templates/includes/dashboard-filter.html Outdated Show resolved Hide resolved
static/scss/app.scss Outdated Show resolved Hide resolved
static/js/filter.js Outdated Show resolved Hide resolved
static/js/app.js Outdated Show resolved Hide resolved
static/scss/pages/dashboard.scss Show resolved Hide resolved
privaterelay/templates/includes/alias-card.html Outdated Show resolved Hide resolved
static/scss/pages/dashboard.scss Show resolved Hide resolved
static/scss/pages/dashboard.scss Outdated Show resolved Hide resolved
privaterelay/templates/includes/alias-card.html Outdated Show resolved Hide resolved
@maxxcrawford maxxcrawford requested a review from Vinnl February 23, 2022 23:23
@maxxcrawford maxxcrawford changed the title Critical Emails UI Block Promotional Emails UI Feb 23, 2022
Vinnl
Vinnl approved these changes Feb 24, 2022
View reviewed changes
Copy link
Collaborator

@Vinnl Vinnl left a comment

Choose a reason for hiding this comment

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

Cool, I'm no longer seeing the inconsistent states, so once the l10n strings land, I think this is good to go!

privaterelay/templates/includes/alias-card.html Show resolved Hide resolved
privaterelay/templates/includes/alias-card.html Outdated Show resolved Hide resolved
static/js/app.js Outdated Show resolved Hide resolved
@maxxcrawford maxxcrawford force-pushed the mpp-1347-critical-emails-ui branch 2 times, most recently from cfcb9e4 to 2aba167 Compare February 25, 2022 14:57
maxxcrawford and others added 12 commits February 25, 2022 11:10
maxxcrawford and others added 8 commits February 25, 2022 11:10
@maxxcrawford @Vinnl
Remove TODO comment
1693863 
Co-authored-by: Vincent <Vinnl@users.noreply.github.com>
@maxxcrawford @Vinnl
Update static/scss/pages/dashboard.scss
b3b7972 
Remove commented SCSS

Co-authored-by: Vincent <Vinnl@users.noreply.github.com>
@maxxcrawford
Code review revisions:
237e782 
- Update all "critical" terms to use "promo-blocking"
- Removed commented code/debug classes/unreachable CSS
- Fix bug where aliases were not being set to promo-blocking on page load
- Add additional context in comments
- Fix bug where filter would show extra aliases when filtering by promo-blocking only.
- Revised JS code to be more readable
@maxxcrawford
Update ul/li to output element
2ee0abe
@maxxcrawford
Update changeEvent to use form target attribute
a7740e1
@maxxcrawford
UNRELATED: Fix issue with CSAT form error in analytics.js
41fde40
@maxxcrawford
Fix issue where fonts were not being located correctly by Protocol li…
12a1b8c 
…brary, added missing fonts
@maxxcrawford
Submodule updates
1523866
@maxxcrawford maxxcrawford force-pushed the mpp-1347-critical-emails-ui branch from bc1c0b5 to 1523866 Compare February 25, 2022 17:11
@maxxcrawford maxxcrawford merged commit 6eeca71 into main Feb 25, 2022
@maxxcrawford maxxcrawford deleted the mpp-1347-critical-emails-ui branch June 10, 2022 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Vinnl Vinnl Vinnl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@maxxcrawford @Vinnl