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

VPN-4690: Add excluded apps help sheet #8879

Merged
merged 3 commits into from
Jan 19, 2024
Merged

VPN-4690: Add excluded apps help sheet #8879

merged 3 commits into from
Jan 19, 2024

Conversation

MattLichtenstein
Copy link
Contributor

Description

NOTE: Depends on #8864: VPN-4687: Add DNS settings help sheet

  • Implement help sheet for excluded apps
Screen.Recording.2023-12-29.at.2.44.02.PM.mov

Reference

VPN-4690: Implement the 'App Exclusions' in-product help sheet

Copy link
Collaborator

@flodolo flodolo left a comment

Choose a reason for hiding this comment

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

A couple of questions for English content, only the first really affecting localization.

value: What are excluded apps?
comment: Header label for the excluded apps help sheet
excludedAppsBody1:
value: Excluded apps let you turn off VPN protection for specific apps, without turning off your device’s VPN protection.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it just me, or this can be confusing? It makes it sound like the device has a VPN protection that is unrelated to Mozilla VPN, unlike something like this:

… without turning off VPN protection for your entire device.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Sure, we can make this change. Let's update to Flod's suggestion:

Excluded apps let you turn off VPN protection for specific apps, without turning off VPN protection for your entire device.

value: Excluded apps let you turn off VPN protection for specific apps, without turning off your device’s VPN protection.
comment: Body label for the excluded apps help sheet
excludedAppsBody2:
value: When you exclude an app, your IP address and approximate location will be exposed to it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
value: When you exclude an app, your IP address and approximate location will be exposed to it.
value: When you exclude an app, it will have access to your IP address and approximate location.

In case content needs to check these strings, this feels a bit easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Sure, I also like this suggestion.

Thanks Flod.

Copy link
Member

@lesleyjanenorton lesleyjanenorton left a comment

Choose a reason for hiding this comment

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

This looks great! 🤩 One thing to change in the URL but other comments are just nits.

@@ -1381,6 +1381,11 @@ void MozillaVPN::registerUrlOpenerLabels() {
uo->registerUrlLabel("upgradeToAnnualUrl", []() -> QString {
return Constants::upgradeToAnnualUrl();
});

uo->registerUrlLabel("sumoExcludedApps", []() -> QString {
return "https://support.mozilla.org/en-US/kb/"
Copy link
Member

Choose a reason for hiding this comment

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

let's drop the "en-US" here because it stipulates that the page is shown in English. Without it, moz.org will route the page based on the person's browser language prefs.

title: MZI18n.HelpSheetsExcludedAppsTitle

model: [
{type: MZHelpSheet.BlockType.Title, text: MZI18n.HelpSheetsExcludedAppsHeader},
Copy link
Member

Choose a reason for hiding this comment

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

nit: magic numbers

Copy link
Contributor Author

@MattLichtenstein MattLichtenstein Jan 18, 2024

Choose a reason for hiding this comment

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

will add to theme.js to be used by all help sheets in a fast follow

model: [
{type: MZHelpSheet.BlockType.Title, text: MZI18n.HelpSheetsExcludedAppsHeader},
{type: MZHelpSheet.BlockType.Text, text: MZI18n.HelpSheetsExcludedAppsBody1, margin: 8},
{type: MZHelpSheet.BlockType.Text, text:MZI18n.HelpSheetsExcludedAppsBody2, margin: 16},
Copy link
Member

Choose a reason for hiding this comment

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

text:MZI18n.HelpSheetsExcludedAppsBody2

nit: space between 'text:' and 'MZ...'

@MattLichtenstein MattLichtenstein merged commit 3b02e24 into main Jan 19, 2024
125 checks passed
@MattLichtenstein MattLichtenstein deleted the VPN-4690 branch January 19, 2024 00:02
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.

5 participants