Automattic / wc-calypso-bridge

20 stars 4 forks source link

i18n: Load script translations #1472

Closed yuliyan closed 3 months ago

yuliyan commented 3 months ago

Changes proposed in this Pull Request:

Related to 799-gh-Automattic/i18n-issues

How to test the changes in this Pull Request:

  1. Checkout the branch locally.
  2. Generate JSON translation files for one of the languages with wp i18n make-json languages/[language].po.
  3. Sync the changes to your test site by following the instructions from pdDOJh-3ob-p2.
  4. Open your test site and make sure set to the language you've generated the translation files for.
  5. Navigate to /wp-admin/admin.php?page=wc-admin and confirm wc-calypso-bridge-js-translations translation data is loaded, i.e. the script <script id="wc-calypso-bridge-js-translations"> that loads the data using wp.i18n.setLocaleData() is printed on the page.
  6. Navigate to /wp-admin/admin.php?page=wc-admin&path=%2Fmarketing and confirm translations for lazy loaded module Marketing are loaded as expected. The isEcommercePlan, isEcommercePlanTrial, isWooNavigationEnabled params on your test site must be true (can be overridden in class-wc-calypso-bridge-shared.php)

Other information:

FOR PR REVIEWER ONLY:

github-actions[bot] commented 3 months ago

Size Change: +2.8 kB (+1.42%)

Total Size: 200 kB

Filename Size Change
./build/index.js 125 kB +2.8 kB (+2.29%)
ℹ️ View Unchanged | Filename | Size | | :--- | :---: | | `./build/53.js` | 1.05 kB | | `./build/index.css` | 883 B | | `./build/marketing.js` | 58 kB | | `./build/payment-gateway-suggestions.css` | 1.25 kB | | `./build/payment-gateway-suggestions.js` | 6.46 kB | | `./build/plugins.js` | 3.92 kB | | `./build/style-index.css` | 2.15 kB | | `./build/style-marketing.css` | 805 B |

compressed-size-action

yuliyan commented 3 months ago

Thank your for reviewing the changes, @ilyasfoo!

I'm sorry I didn't make the instructions more clear, but rendering the translated strings is a separate problem caused by using incorrect text domain (i.e. woocommerce instead of wc-calypso-bridge).

The goal of this PR is to load the translation data into the i18n instance, i.e. to make sure wc-calypso-bridge-js-translations script is printed on the page.

I plan to follow up with creating a separate PR to update the text domain and another PR to commit translations files (i.e. run npm run prepare).

yuliyan commented 3 months ago

@ilyasfoo,

I've previously mentioned that some gettext calls were using incorrect text , but after further looking into it, I've noticed that some of the strings also exist in WooCommerce and updating the text domain would it from re-using the WooCommerce translations.

Instead, I took a different approach and add fallback translation lookup for gettext calls with woocommerce text domain that are missing tranaslations - https://github.com/Automattic/wc-calypso-bridge/pull/1472/commits/f1bceadbd63ad7006885325029e469a92f8c02bd. If you repeat your testing steps, you should now see the translated items.

I've also noticed that there were some lazy loaded modules that have their own translation files, that were previously not handled, so I've updated the PR to also handle this case in https://github.com/Automattic/wc-calypso-bridge/pull/1472/commits/9f81bc2882b65eaecebf8922bb6c1153c53e73ec.