-
Notifications
You must be signed in to change notification settings - Fork 109
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-6580 change 'App Exclusions' to 'Excluded Apps' #9879
Conversation
@@ -154,7 +154,7 @@ The excluded apps list will likely live in the client repo. There are ways to we | |||
Considerations for roll out | |||
---------------------- | |||
|
|||
While the reworked app exclusion page will affect all platforms that support split tunneling, notifications will only be shown to users who have already allowed our app to send them. On mobile, we show the "approve notifications" screen immediately upon launching the app for the first time, which likely depresses approval rates. We may want to re-consider how we show this as part of onboarding. | |||
While the reworked exclulded apps page will affect all platforms that support split tunneling, notifications will only be shown to users who have already allowed our app to send them. On mobile, we show the "approve notifications" screen immediately upon launching the app for the first time, which likely depresses approval rates. We may want to re-consider how we show this as part of onboarding. |
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.
typo :)
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.
Thanks, fixed
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.
Hmmmm... i thought the point of these that they are historic docs (once accepted) not docs we need to keep up to date for ever 😅 Appreciate it tho :)
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.
Yeah, wasn't sure that was strictly necessary (and I wouldn't ding someone else's review for not including it), but it came up in a global search for the term so I figured it would help it be clearer for the future. (Though had I changed logic relating to that doc, I wouldn't have updated the doc. 🤷🏻 )
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.
Is it just me or we keep going back and forth on the terminology for this feature? Was the impact on SuMO accounted for here?
We still have strings in Pontoon referencing Excluded apps
from the previous round.
infoLinkText: Learn more | ||
infoCardDescription: | ||
value: VPN must be off to edit App exclusions | ||
comment: Displays inside the information card at the top of the app exclusions screen when the VPN is on | ||
value: VPN must be off to edit Excluded apps |
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.
To change an existing string you need a new ID.
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.
I wrote it that way initially. But do you think the translations for "App exclusions" and "Excluded apps" will be substantially different? If not, perhaps we don't need to bother the translators with this one. I'll defer to you.
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.
In general, if we think a fix it's worth for English (for consistency), we also want it in other locales. The translations will be definitely different between the two.
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.
Sounds good, I've updated the PR to use new string IDs.
src/translations/strings.yaml
Outdated
@@ -323,7 +318,7 @@ systray: | |||
|
|||
settings: | |||
privacySettings: Privacy features | |||
appExclusionSettings: App exclusions | |||
appExclusionSettings: Excluded apps |
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.
Same here, needs a new ID.
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.
Per above, these now have new IDs.
I'll let Santiago speak to the terminology updates on this. I know some places (the widget?) already use |
This reverts commit 4454598.
@flodolo Yes, the SUMO changes are accounted for here: |
Description
A quick string change.
infoText2
, so I removed that string.Reference
VPN-6580
Checklist