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

Add clearer tracker removal breakage warnings #2301

Merged
merged 3 commits into from
Aug 9, 2022
Merged

Conversation

Vinnl
Copy link
Collaborator

@Vinnl Vinnl commented Aug 4, 2022

This PR fixes MPP-2248. The news announcement will also need an update, but I haven't received that string yet has been updated as well.

How to test: look at the locations I screenshotted in the l10n PR - the text should be updated to mention trackers in images and links, and about how broken emails can't be recovered.

@Vinnl Vinnl added the Review: µ Code review time: 5 minutes or less label Aug 4, 2022
@Vinnl Vinnl requested review from lloan and codemist August 4, 2022 11:54
@Vinnl Vinnl self-assigned this Aug 4, 2022
Copy link
Contributor

@lloan lloan left a comment

Choose a reason for hiding this comment

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

LGTM

# This is displayed in small under a number in a large font indicating the number of trackers that have been removed from all emails sent to a particular mask
profile-label-trackers-removed = Trackers removed
profile-trackers-removed-tooltip-part1 = With tracker removal enabled, common email trackers will be removed from your forwarded emails.
profile-trackers-removed-tooltip-part2 = Important: Sometimes removing trackers may cause your email to look broken, because the trackers are often contained within images.
profile-trackers-removed-tooltip-part2-2 = <b>Important:</b> Removing trackers may cause your email to look broken, because the trackers are often contained within images and links.
Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocking: This is new to me - I did not know we could include html within strings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it's kinda cumbersome at the moment since you can't use the useString hook - you'll need to use <Localized>. (Incidentally, I have a pull request open on @fluent/react to provide a similar hook API for it.) It's also not exactly HTML; I could've used anything tag-like here (e.g. <notice>Important:</notice>, and then in the elems attribute of <Localized> you can provide a mapping of that to actual tags.

@lloan lloan merged commit 1997c8f into main Aug 9, 2022
@lloan lloan deleted the MPP-2248-breakage-warnings branch August 9, 2022 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: µ Code review time: 5 minutes or less
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants