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

Use a newer version of the WP-CLI command to include parsing issue fixes #4323

Merged
merged 11 commits into from
Dec 2, 2021

Conversation

fluiddot
Copy link
Contributor

@fluiddot fluiddot commented Dec 1, 2021

The parsing issues we've been experiencing when generating the JS bundle in previous releases are now solved on trunk branch of WP-CLI repository. Until a new version is released (potentially 2.5.1), we could use the nightly version in the meantime to prevent the known issues related to generating the localization string files when running npm run bundle.

NOTE: The Gutenberg reference is pointing to a Gutenberg PR that includes a change in the string Add block, in order to reuse the translation from Gutenberg and prevent adding a new one in the localization string files.

Note for reviewers: The PR modified a lot of files but most of them are the translation cache files updated due to force cache update command. They are located in the following paths, feel free to skip them:

  • src/i18n-cache/gutenberg/data/*.json
  • bundle/ios/assets/gutenberg/packages/react-native-editor/i18n-cache/data/*.json

Feel free to skip them as

To test:

Verify that the localization strings files are not modified after generating the JS bundle

Review that string additions and removals are expected in the PR

NOTE: I've done the first review in this comment.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

@fluiddot fluiddot added this to the 1.68.0 (18.9) milestone Dec 1, 2021
@fluiddot fluiddot self-assigned this Dec 1, 2021
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Dec 1, 2021

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

Comment on lines -77 to +78
"makepot:android": "npm run wp -- i18n make-pot ./gutenberg/packages --include=*.native.js,*.android.js --exclude=test/*,e2e-tests/*,build/*,build-module/*,build-style/*,*.js,*.php --subtract=./gutenberg.pot --ignore-domain gutenberg-android.pot",
"makepot:ios": "npm run wp -- i18n make-pot ./gutenberg/packages --include=*.native.js,*.ios.js --exclude=test/*,e2e-tests/*,build/*,build-module/*,build-style/*,*.js,*.php --subtract=./gutenberg.pot --ignore-domain gutenberg-ios.pot",
"makepot:android": "npm run wp -- i18n make-pot ./gutenberg/packages --include=*.native.js,*.android.js --exclude=test/*,e2e-tests/*,build/*,build-module/*,build-style/* --skip-php --subtract=./gutenberg.pot --ignore-domain gutenberg-android.pot",
"makepot:ios": "npm run wp -- i18n make-pot ./gutenberg/packages --include=*.native.js,*.ios.js --exclude=test/*,e2e-tests/*,build/*,build-module/*,build-style/* --skip-php --subtract=./gutenberg.pot --ignore-domain gutenberg-ios.pot",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The include parameter now works as expected and it's not required to explicitly exclude *.js files. Additionally, for excluding the PHP files, I added the -skip-php argument.

As part of the bundling process, we should re-download and update the i18n cache files to provide the most recent translations.
@@ -64,18 +64,18 @@
"i18n:gutenberg": "./bin/i18n-download.sh gutenberg",
"i18n:gutenberg:force": "cross-env REFRESH_I18N=1 ./bin/i18n-download.sh gutenberg",
"bundle": "npm run clean; npm run bundle:js && npm run genstrings",
"prebundle:js": "npm run i18n:gutenberg",
"prebundle:js": "npm run i18n:gutenberg:force",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As part of the bundling process, we should re-download and update the i18n cache files to provide the most recent translations.

This behavior is identical to the JS bundle generation in Gutenberg (reference). We haven't updated the cache since a long time ago because the translations from the GlotPress project was missing some of the strings (more info here), now we can re-enable it as the issue was fixed on previous versions.

Copy link
Contributor Author

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

I've reviewed the string modifications from bundle/ios/GutenbergNativeTranslations.swift (contains the same changes as in bundle/android/strings.xml) and added a comment on each one regarding the cause of the modification.

@@ -2,6 +2,7 @@ import Foundation

private func dummy() {
_ = NSLocalizedString("'%s' is not fully-supported", comment: "translators: Missing block alert title. %s: The localized block name")
_ = NSLocalizedString("%1$s (%2$s)", comment: "translators: %1$s: Select control font size name e.g. Small, %2$s: Select control font size e.g. 12px")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we missed this string, most likely this was caused due to the parsing issues when extracting the strings.

@@ -28,7 +29,6 @@ private func dummy() {
_ = NSLocalizedString("ADD IMAGE", comment: "")
_ = NSLocalizedString("Add image or video", comment: "")
_ = NSLocalizedString("ADD IMAGE OR VIDEO", comment: "")
_ = NSLocalizedString("Add link", comment: "")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This string is already translated as it's used in the web version, so we don't need to include it here.

@@ -77,6 +77,7 @@ private func dummy() {
_ = NSLocalizedString("Clear search", comment: "")
_ = NSLocalizedString("Column %d", comment: "translators: %d: column index.")
_ = NSLocalizedString("Columns Settings", comment: "")
_ = NSLocalizedString("Contact support", comment: "")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we missed this string, most likely this was caused due to the parsing issues when extracting the strings.

@@ -95,7 +96,6 @@ private func dummy() {
_ = NSLocalizedString("Cut block", comment: "")
_ = NSLocalizedString("Describe the purpose of the image. Leave empty if the image is purely decorative. ", comment: "")
_ = NSLocalizedString("Dismiss", comment: "")
_ = NSLocalizedString("Display post date", comment: "")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This string is already translated as it's used in the web version, so we don't need to include it here.

@@ -122,6 +122,8 @@ private func dummy() {
_ = NSLocalizedString("Double tap to select a video", comment: "")
_ = NSLocalizedString("Double tap to select an audio file", comment: "")
_ = NSLocalizedString("Double tap to select an image", comment: "")
_ = NSLocalizedString("Double tap to select default font size", comment: "")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we missed this string, most likely this was caused due to the parsing issues when extracting the strings.

@@ -223,8 +223,6 @@ private func dummy() {
_ = NSLocalizedString("Page title. Empty", comment: "translators: accessibility text. empty page title.")
_ = NSLocalizedString("Paste block after", comment: "")
_ = NSLocalizedString("Paste URL", comment: "")
_ = NSLocalizedString("Post content settings", comment: "")
_ = NSLocalizedString("Post meta settings", comment: "")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This string is already translated as it's used in the web version, so we don't need to include it here.

Comment on lines -255 to +253
_ = NSLocalizedString("Selected: %s", comment: "translators: %s: Select control option value e.g: \"Auto, 25%\".")
_ = NSLocalizedString("Selected: %s", comment: "translators: %s: Select font size option value e.g: \"Selected: Large\".\ntranslators: %s: Select control option value e.g: \"Auto, 25%\".")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This string is used in two places and a different comment is provided from each one, for this reason, the value of the comment property contains two lines.

@@ -252,19 +250,20 @@ private func dummy() {
_ = NSLocalizedString("Select a color", comment: "")
_ = NSLocalizedString("Select a color above", comment: "")
_ = NSLocalizedString("Select a layout", comment: "")
_ = NSLocalizedString("Selected: %s", comment: "translators: %s: Select control option value e.g: \"Auto, 25%\".")
_ = NSLocalizedString("Selected: %s", comment: "translators: %s: Select font size option value e.g: \"Selected: Large\".\ntranslators: %s: Select control option value e.g: \"Auto, 25%\".")
_ = NSLocalizedString("Selected: Default", comment: "")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we missed this string, most likely this was caused due to the parsing issues when extracting the strings.

_ = NSLocalizedString("Set as Featured Image ", comment: "")
_ = NSLocalizedString("Show post content", comment: "")
_ = NSLocalizedString("Slash inserter results", comment: "translators: Slash inserter autocomplete results")
_ = NSLocalizedString("Some blocks have additional settings. Tap the settings icon on the bottom right of the block to view more options.", comment: "")
_ = NSLocalizedString("Sorting and filtering", comment: "")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This string is already translated as it's used in the web version, so we don't need to include it here.

_ = NSLocalizedString("Start writing…", comment: "")
_ = NSLocalizedString("Take a Photo", comment: "")
_ = NSLocalizedString("Take a Photo or Video", comment: "")
_ = NSLocalizedString("Take a Video", comment: "")
_ = NSLocalizedString("Tap here to show help", comment: "")
_ = NSLocalizedString("Tap to hide the keyboard", comment: "")
_ = NSLocalizedString("Text formatting controls are located within the toolbar positioned above the keyboard while editing a text block", comment: "")
_ = NSLocalizedString("The basics", comment: "")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we missed this string, most likely this was caused due to the parsing issues when extracting the strings.

@fluiddot fluiddot marked this pull request as ready for review December 1, 2021 17:14
@fluiddot fluiddot requested review from SiobhyB and dcalhoun December 1, 2021 17:15
Copy link
Contributor

@SiobhyB SiobhyB left a comment

Choose a reason for hiding this comment

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

I verified each of the changes to bundle/ios/GutenbergNativeTranslations.swift (thank you for making it so straightforward with your annotations and code links for each!) I then also confirmed the changes to bundle/android/strings.xml matched these and that there are no changes after running npm run bundle.

LGTM! 🚀

@fluiddot fluiddot merged commit 6312816 into develop Dec 2, 2021
@fluiddot fluiddot deleted the update/use-nightly-version-wp-cli branch December 2, 2021 14:45
@fluiddot fluiddot mentioned this pull request Dec 9, 2021
2 tasks
@guarani guarani mentioned this pull request Dec 9, 2021
4 tasks
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.

2 participants