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

Update Privacy Pro translations #3656

Merged
merged 25 commits into from
Dec 12, 2024
Merged

Conversation

miasma13
Copy link
Contributor

Task/Issue URL: https://app.asana.com/0/0/1208750072186506/f
CC: @SabrinaTardio

Description:
Update Privacy Pro and VPN related translations.

Optional E2E tests:

  • Run PIR E2E tests
    Check this to run the Personal Information Removal end to end tests. If updating CCF, or any PIR related code, tick this.

Steps to test this PR:
Change preferred language for the app and verify completeness for:

  • Privacy Pro settings when not signed in
  • Privacy Pro settings when signed in
  • VPN settings and popover views

Definition of Done:


Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

Copy link
Contributor

github-actions bot commented Dec 10, 2024

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.
Messages
📖

You seem to be updating localized strings. Make sure that you request translations and include translated strings before you ship your change. See Localization Guidelines for more information.

Generated by 🚫 dangerJS against d30b5d5

Comment on lines +24 to +25
static let networkProtectionStatusMenuSendFeedback = NSLocalizedString("dbp.menu.send.feedback", value: "Send Feedback…", comment: "The status menu 'Send Feedback' menu item")
static let networkProtectionStatusMenuOpenDuckDuckGo = NSLocalizedString("dbp.open.duckduckgo", value: "Open DuckDuckGo…", comment: "The status menu 'Open DuckDuckGo' menu item")
Copy link
Collaborator

@samsymons samsymons Dec 11, 2024

Choose a reason for hiding this comment

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

Something is strange here - it's a Network Protection string, but in a DBP file, with a DBP prefix. At the end of the day it should work as intended, but caught me off guard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it was a copy paste issue and fixed the string ID but completely missed the variable name. I see it is not used anywhere, maybe it was a placeholder to setup localizations. Since they are not used anywhere I will proceed and delete it.

Copy link
Collaborator

@samsymons samsymons left a comment

Choose a reason for hiding this comment

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

Overall this looks good in most areas, I noticed that the VPN settings UI is largely untranslated and that some strings in the submenus are missing though. I recall some of these were still going through translation, so apologies if I'm pointing out the obvious - just capturing all issues I see to be safe.

CleanShot 2024-12-10 at 19 06 12@2x

CleanShot 2024-12-10 at 19 06 33@2x

CleanShot 2024-12-10 at 19 06 51@2x

@samsymons
Copy link
Collaborator

One other view that I noticed is missing translation - when opening PIR in a debug build, I get prompted to move the app, but it's still using English.

CleanShot 2024-12-10 at 19 23 45@2x

@miasma13
Copy link
Contributor Author

Thanks for the review @samsymons. Regarding the VPN translations I have last translation job to import today, and I will double check the PIR message.

Copy link
Collaborator

@SabrinaTardio SabrinaTardio left a comment

Choose a reason for hiding this comment

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

Remember to add English (United States) as language to the Localizable files of the new packages (as done in SyncUI).
This is to get around an apple issue.

@miasma13
Copy link
Contributor Author

Applied all suggested changes. The DBP strings happened to be NotLocalizedStrings - changed them and we are running one most final translation job 🤞. After importing them this will be ready to be merged.

@miasma13 miasma13 merged commit bd4493f into release/1.118.0 Dec 12, 2024
30 checks passed
@miasma13 miasma13 deleted the michal/update-pp-translations branch December 12, 2024 15:19
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.

3 participants