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

i18n: Remove redundant hasTranslation checks (across the codebase) #98096

Merged
merged 3 commits into from
Jan 9, 2025

Conversation

chriskmnds
Copy link
Contributor

Related to https://github.com/Automattic/i18n-issues/issues/654

Proposed Changes

  • Part of addressing https://github.com/Automattic/i18n-issues/issues/654 and follow-up to i18n: Introduce fixMe method to i18n-calypso #97690
  • Initially I meant to go through the codebase and replace the various hasTranslation + locale checks with the new fixMe method, but all of the current cases are effectively redundant checks (meaning, translations have been completed), so instead created a wholistic PR to remove the checks and fallback strings all across the codebase.
  • The refactor only includes cases where the hasTranslation + locale check was done to choose between an original and new translation. Some other cases exist (I think 2-3) where a component will not render at all if a translation is not present. I left those as-is for now - might do in a follow-up.

This is a risky PR as it touches many files.

Why are these changes being made?

  • Part of addressing https://github.com/Automattic/i18n-issues/issues/654
  • Removes a lot of redundant conditioning from the codebase
  • Creates a new base from which we can more directly introduce/suggest usage of the new fixme method i.e. removes any precedent of hasTranslation + locale check.
  • This is a wholistic and risky PR to just get this out of the way. The alternative would have been something like 40 or so individual PRs (never gonna happen).

Testing Instructions

  • Code-review as detailed as possible.
  • For functional testing:
    • I don't think it's effective to attempt to test all these cases one by one. We can instead get the PR in a working/shippable state and let experience guide us / and allow the risk of regressions, which we can address in follow-ups. 🤷‍♂️
  • For translations:
    • For each of the original/new combinations, confirm (I have confirmed most if not all of these):
      • Go to GlotPress and confirm the translations across locales between the original and the new string are equivalent.

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
    • For UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

@chriskmnds chriskmnds added the i18n label Jan 8, 2025
@chriskmnds chriskmnds requested a review from a team January 8, 2025 17:08
@chriskmnds chriskmnds self-assigned this Jan 8, 2025
@chriskmnds chriskmnds requested review from a team as code owners January 8, 2025 17:08
@chriskmnds chriskmnds requested a review from jeyip January 8, 2025 17:08
@matticbot matticbot added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 8, 2025
@matticbot
Copy link
Contributor

matticbot commented Jan 8, 2025

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~336 bytes removed 📉 [gzipped])

name           parsed_size           gzip_size
entry-login        -1711 B  (-0.1%)     -336 B  (-0.1%)
entry-main         -1143 B  (-0.1%)     -218 B  (-0.0%)
entry-stepper       -294 B  (-0.0%)      -69 B  (-0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~2412 bytes removed 📉 [gzipped])

name                                parsed_size           gzip_size
domains                                 -1468 B  (-0.1%)     -285 B  (-0.0%)
hosting                                 -1413 B  (-0.1%)     -290 B  (-0.0%)
site-tools                              -1398 B  (-0.1%)     -247 B  (-0.0%)
plans                                   -1275 B  (-0.1%)     -247 B  (-0.1%)
plugin-bundle-flow                      -1248 B  (-0.2%)     -199 B  (-0.1%)
update-design-flow                      -1042 B  (-0.1%)     -197 B  (-0.0%)
link-in-bio-tld-flow                    -1042 B  (-0.0%)     -197 B  (-0.0%)
copy-site-flow                          -1042 B  (-0.1%)     -197 B  (-0.1%)
staging-site                            -1040 B  (-0.1%)     -152 B  (-0.0%)
subscribers                              -929 B  (-0.1%)     -199 B  (-0.1%)
write-flow                               -849 B  (-0.1%)     -149 B  (-0.0%)
site-setup-wg                            -849 B  (-0.2%)     -149 B  (-0.1%)
site-setup-flow                          -849 B  (-0.2%)     -149 B  (-0.1%)
new-hosted-site-flow-user-included       -849 B  (-0.5%)     -149 B  (-0.3%)
new-hosted-site-flow                     -849 B  (-0.5%)     -149 B  (-0.3%)
import-hosted-site-flow                  -849 B  (-0.1%)     -149 B  (-0.0%)
import-flow                              -849 B  (-0.5%)     -149 B  (-0.3%)
hundred-year-plan                        -849 B  (-0.4%)     -149 B  (-0.3%)
hundred-year-domain                      -849 B  (-0.4%)     -149 B  (-0.2%)
connect-domain                           -849 B  (-0.4%)     -149 B  (-0.3%)
build-flow                               -849 B  (-0.1%)     -149 B  (-0.0%)
site-purchases                           -834 B  (-0.1%)     -110 B  (-0.0%)
purchases                                -834 B  (-0.0%)     -110 B  (-0.0%)
site-settings                            -828 B  (-0.1%)     -206 B  (-0.0%)
settings                                 -690 B  (-0.1%)     -151 B  (-0.0%)
home                                     -598 B  (-0.0%)     -110 B  (-0.0%)
patterns                                 -540 B  (-0.0%)     -152 B  (-0.0%)
github-deployments                       -496 B  (-0.0%)     -148 B  (-0.0%)
settings-jetpack                         -492 B  (-0.1%)      -60 B  (-0.0%)
scan                                     -492 B  (-0.1%)      -59 B  (-0.0%)
jetpack-social                           -492 B  (-0.1%)      -59 B  (-0.0%)
jetpack-search                           -492 B  (-0.1%)      -59 B  (-0.0%)
jetpack-connect                          -492 B  (-0.0%)      -59 B  (-0.0%)
jetpack-cloud-settings                   -492 B  (-0.1%)      -60 B  (-0.0%)
jetpack-cloud-pricing                    -492 B  (-0.1%)      -59 B  (-0.0%)
jetpack-cloud-overview                   -492 B  (-0.1%)      -65 B  (-0.0%)
jetpack-cloud-manage-pricing             -492 B  (-0.1%)      -59 B  (-0.1%)
jetpack-cloud-features-comparison        -492 B  (-0.1%)      -59 B  (-0.0%)
jetpack-cloud-agency-sites-v2            -492 B  (-0.0%)      -59 B  (-0.0%)
backup                                   -492 B  (-0.0%)      -59 B  (-0.0%)
a8c-for-agencies-sites                   -492 B  (-0.0%)      -59 B  (-0.0%)
settings-newsletter                      -460 B  (-0.1%)     -108 B  (-0.1%)
transferring-hosted-site-flow            -399 B  (-0.1%)      -81 B  (-0.1%)
marketplace                              -323 B  (-0.0%)      -83 B  (-0.0%)
plugins                                  -283 B  (-0.0%)      -73 B  (-0.0%)
jetpack-cloud-plugin-management          -283 B  (-0.0%)      -73 B  (-0.0%)
notification-settings                    -260 B  (-0.0%)      -57 B  (-0.0%)
account                                  -225 B  (-0.0%)      -64 B  (-0.0%)
developer                                -139 B  (-0.0%)      -42 B  (-0.0%)
sites-dashboard                          -138 B  (-0.0%)      -53 B  (-0.0%)
site-performance                         -138 B  (-0.0%)      -53 B  (-0.0%)
site-overview                            -138 B  (-0.0%)      -53 B  (-0.0%)
site-monitoring                          -138 B  (-0.0%)      -53 B  (-0.0%)
site-marketing                           -138 B  (-0.0%)      -53 B  (-0.0%)
site-logs                                -138 B  (-0.0%)      -53 B  (-0.0%)
hosting-features                         -138 B  (-0.0%)      -53 B  (-0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~451 bytes removed 📉 [gzipped])

name                                        parsed_size           gzip_size
async-load-automattic-help-center-stepper       -1163 B  (-0.1%)     -266 B  (-0.1%)
async-load-calypso-my-sites-checkout-modal       -849 B  (-0.1%)     -149 B  (-0.0%)
async-load-signup-steps-site-or-domain           -796 B  (-1.1%)     -112 B  (-0.5%)
async-load-automattic-help-center                -314 B  (-0.0%)     -117 B  (-0.1%)
async-load-signup-steps-domains                  -193 B  (-0.0%)      -48 B  (-0.0%)
async-load-signup-steps-design-picker             -81 B  (-0.1%)      -25 B  (-0.1%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Member

@alshakero alshakero left a comment

Choose a reason for hiding this comment

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

Beautiful work. Thank you!!

I checked every file one by one and all look good. Found two tiny issues. I would add String Freeze label to see if the i18n bot detects any new strings (bad sign).

Comment on lines +33 to +35
translate(
'When enabled, only posts published under the categories selected below will be emailed to your subscribers.'
) }
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated but weird concat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed weird. Maybe they wanted to add to existing translation without modifying it 🤔

Comment on lines -558 to -560
return i18n.getLocaleSlug() === 'en' ||
i18n.hasTranslation( 'Choose how to use your domains' )
? i18n.translate( 'Choose how to use your domain', 'Choose how to use your domains', {
Copy link
Member

Choose a reason for hiding this comment

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

Huh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah. conditional plural or something. Amazing how much we leave lingering this way


return __( 'Contact WordPress.com Support (English)', __i18n_text_domain__ );
}, [ __, isEnglishLocale ] );
return __( 'Contact WordPress.com Support', __i18n_text_domain__ );
Copy link
Member

Choose a reason for hiding this comment

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

I'd drop the memo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good eye! Done


return __( 'Quick response:', __i18n_text_domain__ );
}, [ __, isEnglishLocale ] );
return __( 'AI Generated Response:', __i18n_text_domain__ );
Copy link
Member

Choose a reason for hiding this comment

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

+1

@alshakero alshakero added the [Status] String Freeze Add the [Status] String Freeze label to your PR to ensure new strings are translated before merging label Jan 9, 2025
@chriskmnds
Copy link
Contributor Author

Ohh thank you @alshakero ! You saved me from the hassle to split the PR in chunks to attract reviews. Much appreciate it ❤️

I'll address all the comments of course.

@chriskmnds chriskmnds force-pushed the update/i18n-remove-has-translation-checks branch from a94fdbd to a1a6112 Compare January 9, 2025 15:50
@chriskmnds
Copy link
Contributor Author

I would add String Freeze label to see if the i18n bot detects any new strings (bad sign).

Smart! Thank you.

@matticbot
Copy link
Contributor

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • blaze-dashboard
  • command-palette-wp-admin
  • happy-blocks
  • help-center
  • notifications
  • odyssey-stats
  • whats-new

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/i18n-remove-has-translation-checks on your sandbox.

@chriskmnds
Copy link
Contributor Author

alright - everything checks out. no new strings, no failing tests, all green, bunch of manual checking. If anything pops up, we can address.

Thank you @alshakero 🙇

@chriskmnds chriskmnds merged commit 82a7a71 into trunk Jan 9, 2025
11 checks passed
@chriskmnds chriskmnds deleted the update/i18n-remove-has-translation-checks branch January 9, 2025 16:13
@github-actions github-actions bot removed [Status] String Freeze Add the [Status] String Freeze label to your PR to ensure new strings are translated before merging [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants