-
Notifications
You must be signed in to change notification settings - Fork 115
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
[Brand Updates] Add missing periods to Empty State messages #14940
Merged
hichamboushaba
merged 2 commits into
feature/woo-2.0-brand-updates
from
task/woo-2.0-add-periods
Jan 23, 2025
+8
−32
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to remove these strings from
Localizable.strings, since they will be removed automatically during the code freeze.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with localization process for iOS, so please bear with me here 😅.
I did this because I noticed it was done before multiple times for similar cases (history of the file), if I don't update the file like this, then the old Strings defined in the file will keep showing until the localization is done, which would happen at the end of the code freeze if I'm not mistaken, and this would mean releasing the first beta build with the old strings, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two cases here I think, for Strings with defined keys, they won't be removed, they will be updated, and it's for them that my remark above applies.
For the others, I agree, there is no need to remove them, but since we need to remove some, I'm not sure if it's worth it to revert them 🤔.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't worry Hicham, it's understandable if you are not familiar with the process. 😉
Here you can find the instructions on how the localization works in WCiOS.
Basically, you don't need to touch the
Localizable.stringfile. If you delete a string, no other work is needed. While if you update a string, be sure to update the key as well in the code. But also in this last case, you don't need to modify theLocalizable.string.TBH, I would revert the file, just because I wouldn't want this change to create problems for the automatic Localizable file update process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think something is still missing in the discussion, I already checked the above document, and it doesn't talk about the issue here: updating the content of some existing keys, from what I see, in the past other developers resorted to different processes: either update the content in the
Localizable.stringas well, or delete the key, if you prefer updating I can do it too, but it still means touching the file manually, I don't think created any issues in the past, as Glotpress will still download a new updated file at the end of the code freeze.As I shared above, this will mean we'll release the beta at the start of code freeze with the old content, and it would also mean issues during the design review expected before giving the green light for the project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @AliSoftware for the great insights, I don't think what you shared here is common knowledge, and at least in Android, we tend to frequently update the English copies directly in
strings.xmlwhen making just content changes (Strings that placeholders is another topic that I think most developers should be aware of), so I think we should update our docs to explicitly mention that thekey|valuepair should immutable once sent for translation.TIL, can I ask why this the process and why we don't take the originals from
release/*at the start of code freeze? I always thought it was taking fromrelease/*.For this PR specifically, since we are just adding periods to some strings, which won't cause a significant change to translation, I think we should be safe, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I've written all those in a couple of past P2s a while ago, so an MGS search should give you some additional info; I've also made a presentation about all that during the WordPress team meetup in Porto 2022 (with slides and all), and most of that presentation applies to Woo and other products as well, in case that helps better understanding all that.
Well actually you're right, strings are also frozen before we send them to GlotPress:
Localizable.stringsfile is not supposed to be edited by developers and is instead generated usinggenstrings(from the content of the codebase) at the time we do the code-freeze, then the content of theen.lproj/Localizable.stringsshould always reflect the content of the last frozen state. So even if we later backmergerelease/*intotrunkand theen.lproj/Localizable.stringsthat got generated right after the code freeze is back-propagated totrunk, its content is always supposed to reflect what's used in the code that is in therelease/*branch, because that's the only time that file gets regenerated/updated (right after code-freeze)values/strings.xmlcan be modified by devs manually anytime, during code-freeze we make a copy of that file intofastlane/resources/values/strings.xml, which is then the file that gets imported into GlotPress by the wpcom job. This ensures that even if the originalstrings.xmlgets updated post-code-freeze by devs working on features, those changes won't get imported into GlotPress, only the changes that were frozen/copied intofastlane/resources/during code-freeze will (and that file is not supposed to be changed manually either)So in practice the strings that are sent to GlotPress are indeed the ones as they are at the beginning of the
release/*branch right after the code freeze happened.Which in theory means that this should cover the cases where a string is modified in
trunkwhile it has already been sent to GlotPress during the current code-freeze and creation of therelease/*branch: such a change intrunkshould not impact GlotPress after all, at least not until the next code-freeze.But there's still a risk of a key/string that was already previously translated into GlotPress and gets changed later but its new value is only partially translated while the key remains the same… in which case, while I think that GlotPress is able to be smart enough to mark such strings as "needing revision" when it detects the original changed, I fear there's still a risk that an old translation of the old string could still get pulled into the
release/*branch and create an inconsistency…Note
This is not true for all our products, for example Tumblr uses a different approach where they send the strings to the translators continuously from
trunkbecause their code-freeze period is only one week and it'd be too short for the paid translators to translate too many strings in all locales in time if they were only sent on code-freeze Monday to be collected on Thursday to be ready for Friday's submission).So there are products, like Tumblr, for which the risk is higher given the different process, and for which keeping those strings immutable is even more important (Internal ref: paaHJt-51A-p2); while for Woo at least we have less chances of this happening given the strings are only sent to translation once frozen.
That's why our policy about those cases have still always been "better safe than sorry"—especially since we encountered crashes due to mismatching placeholders in different locales/translations due to such cases in the past and encouraging to consider strings to be immutable helps put all the chances on our side to prevent this from happening again in the future.
Yep, if the change is only a typo in the English copy that would not change the meaning / semantics of the copy (e.g. from
"plan_price_subtitle" = "This plan is completely free!"to `"plan_price_subtitle" = "This plan is a paid option, but you can get 10% off the first month.") and that the typo that was present in the English copy is likely not present in the translations (as they would have self-corrected it while translating to the new locale)… then this should be all ok and not cause any trouble.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again Olivier 🙏
Yes, I've checked the P2 that Paolo shared above, and saw also the slides of the presentation you made (thanks btw ❤️), and unless I missed it, I didn't see any explicit mentions about the immutability point, and from my day to day experience, I can tell you the other Android devs are also not aware of this (maybe iOS devs too based on the git history 😅), and that's why I suggested we update our documentation to state this explicitly, I'll try to find some time to handle this for Android.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think at the time I wrote the slides of the presentation (and also at the time some of the past P2s were written) we didn't yet have the rule of thumb to consider strings immutable; it's mostly when we encountered the crashers later—plus seeing how it was an even bigger problem with some other product's processes (like Tumblr)—that we started to enforce this.
@iangmaia I think there have been discussions as well about introducing a Danger check to warn if developers were modifying an existing key as part of a PR? At least it could be an idea to consider. Not sure how easy it'd be—for iOS it'd mostly be detecting manual changes in the
en.lproj/Localizable.stringson any PR other than the code-freeze one, for Android it'd be a bit trickier to detect changes in existing keys (even if the corresponding lines in thestrings.xmlwere also moved to a different line in the file and not just changed in place) all while not detecting additions of new strings as false-positives…—but I'm sure there are ideas to experiment with…There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do recall some ideas around strings immutability. Tthere's this issue already created, but it seems like something else. I think I'll just update it to consider these other possible scenarios as well.