Skip to content

Conversation

@mokagio
Copy link
Contributor

@mokagio mokagio commented Nov 22, 2022

See #19028.

In previous PRs, I only touched the "Dismiss" localized string. In this one, I tried to set a better example and update all the localized strings usages in the context where the "Dismiss" one was.

This should be the last PR in the #19028 series.

Regression Notes

  1. Potential unintended areas of impact – N.A.
  2. What I did to test those areas of impact (or what existing automated tests I relied on) – N.A.
  3. What automated tests I added (or what prevented me from doing so) – N.A.

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes. N.A.
  • I have considered adding accessibility improvements for my changes. N.A.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary. N.A.


private enum Strings {

static let viewsCountDescriptionSingular =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the diff in this this file is too noisy because of the formatting and indentation changes, checkout the isolated commit that adds the reverse-DNS keys 9162d30 (#19635)

Comment on lines +204 to +214
private static let subtitleFormat = NSLocalizedString(
"qrLoginVerifyAuthorization.completedInstructions.subtitle",
value: "Tap '%@' and head back to your web browser to continue.",
comment: "Subtitle instructing the user to tap the dismiss button to leave the log in flow. %@ is a placeholder for the dismiss button name."
)
static let confirmButton = NSLocalizedString(
"qrLoginVerifyAuthorization.completedInstructions.dismiss",
value: "Dismiss",
comment: "Button label that dismisses the qr log in flow and returns the user back to the previous screen"
)
static let subtitle = String(format: subtitleFormat, Self.confirmButton)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my comment in the commit: ea70a12#comments

@mokagio mokagio self-assigned this Nov 22, 2022
@mokagio mokagio added this to the 21.3 milestone Nov 22, 2022
@mokagio mokagio requested a review from crazytonyli November 22, 2022 16:47
@crazytonyli
Copy link
Contributor

Since the localized string keys are changed, I'm guessing the Localizable.string files would also be changed to use the updated keys. But I don't see those files in this PR, is it because we only update them during the release process?

Copy link
Contributor

@crazytonyli crazytonyli left a comment

Choose a reason for hiding this comment

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

:shipit:

Another unrelated note. I did a quick search of how many localized strings are stored as static variables, and the result is 1393, which feels like too many strings to store in memory, it might be valuable to use a computed property instead?

$ ag 'static let \w* = NSLocalizedString' --no-filename | wc -l
    1393

"growAudienceCell.viewsCount.plural",
value: "Views to your site so far",
comment: "Description for view count. Singular."
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to this PR, but it might be worth to explore using .stirngsdict to localize plurals.

@wpmobilebot
Copy link
Contributor

You can test the changes in Jetpack from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19635-ea70a12 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@wpmobilebot
Copy link
Contributor

You can test the changes in WordPress from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19635-ea70a12 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@mokagio
Copy link
Contributor Author

mokagio commented Nov 23, 2022

@crazytonyli

is it because we only update them during the release process?

Yes, as part of the complete_code_freeze lane.

We do that only at code freeze time so that we upload a single batch of new strings to GlotPress.

@mokagio mokagio merged commit c4363cf into trunk Nov 23, 2022
@mokagio mokagio deleted the mokagio/dismiss-reverse-dns branch November 23, 2022 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants