-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Merge changes from beta 19.7.0.1 + Release Notes #18411
Conversation
I had forgotten about this when approving then merging the PR.
…ign-buttons-tweaks [Site Creation] Pass a boolean to determine Site Design primary button titles
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.
- Updates to the localization files (
.strings
and Fastlane metadata) automatically pulled for the new translations that have already been approved in GlotPress - Version update in
.xcconfig
- Diffs from the PRs that made it into this beta
Generated by 🚫 dangerJS |
You can test the changes in WordPress from this Pull Request by:
.ipa file can also be downloaded directly here.If you need access to App Center, please ask a maintainer to add you. |
You can test the changes in Jetpack from this Pull Request by:
.ipa file can also be downloaded directly here.If you need access to App Center, please ask a maintainer to add you. |
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.
Checklist for New Beta
- Changes from bugfix PRs which landed in the release branch
- Version bump in
.xcconfig
files - Latest app and metadata translations pulled from Glotpress
Checklist for Release Notes
- Editorialized Release Notes updated
⚠️ entries fromRELEASE-NOTES.txt
changed in therelease/*
branch 🤔 quite suspicious, and not sure if this was intentional or not, nor if it would require us to revisit the Editorizalized Release Notes accordingly to take that change into account…⚠️ New strings were introduced post-code-freeze in therelease/*
branch (and thus post-strings-freeze too)- Which means those new strings won't be included in the translation pipeline (nor sent to our Translation Vendor nor imported in GlotPress), and thus not translated in time for 19.7.
- @mokagio As a mitigation for this, you might want to consider updating the frozen strings (i.e. run the
generate_strings_file_for_glotpress
again onrelease/19.7
branch) similar to what I decided to do for WPAndroid, so that the new strings from therelease/19.7
branch will be imported into GlotPress. While the WeeklyKit has already been sent and it would be too late for those to get to our translation vendor, this would still give a chance to the polyglots community to help translate those new strings in time for 19.7 submission. - (cc @antonis FYI, as author of those new strings)
* [*] Reader: Fixed a bug that caused cut off content in reader web view [#16106] | ||
* [*] Web previews now abide by safe areas when a toolbar is shown [#18127] |
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.
❓ Is that change expected (especially since that does not seem like just a wording update of a single item, but instead removal of an item related to one PR #16106 while adding a different item related to a different PR #18127)?
(cc @twstokes as author of the commit making that change)
And also, do we need to inform our freelance writer (Grace) about that change and new entry, so that she update her Editorialized Release Notes copy (included in this PR) accordingly?
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.
👋 @AliSoftware - this PR contains the backstory of this change.
Summary:
- The work started out as a fix for Reader: In-app browser preview hides/cuts important elements #16106 by an external contributor
- During the review process we made them aware that the fix had broader scope than just reader
- At the tail end (a little too late) I updated their contributed release note and revised their PR title to better reflect the changes. It was pointed to the issue rather than the PR, so I fixed that as well. I then created [19.7] Update release note for #18127 #18401 to hopefully prevent any surprises on the release side.
Should I have included the Owl Team on that PR to loop everyone in? Any tips welcome! 🙇
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 for the helpful extra context @twstokes !
I think it's always a good idea to ping your Release Manager especially when you change either release notes (like here) or strings (like for the case of the other review comment I left) in a PR that lands in release/*
branch after the code-freeze.
Both release notes and strings are frozen during code freeze and we then use those frozen versions to kick off the rest of the process (ask our freelance writer to write nice notes from this frozen draft, send frozen strings to our translation vendor, …), which is why any change to those after code freeze is important to signal to your RM so they can adjust for those last-minute tweaks appropriately 🙂.
That being said, given the additional context you shared here, I think the wording that ended up being used in the Editorialized version of the Release Notes here below is probably OK and matches what ultimately landed?
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, that makes sense!
I think the wording that ended up being used in the Editorialized version of the Release Notes here below is probably OK and matches what ultimately landed?
I agree!
static let createSiteButton = NSLocalizedString("Create Site", comment: "Title for the button to progress with creating the site with the selected design.") | ||
static let chooseButton = NSLocalizedString("Choose", comment: "Title for the button to progress with the selected site homepage design.") |
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.
release/*
later, those new strings sadly won't be sent for translation and won't be translated in time for 19.7 😞
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.
👋 @AliSoftware thanks for all your information recently about translations. Could I get one bit clarified?
If I understand correctly, "Create Site" and "Choose" are keys* for the translated strings. These keys existed prior to this change in WPiOS and had the same meaning [one] [two].
Does this still mean the translations won't work for this view? I ran it locally and set it to a random language (German) and saw it was translated. Thanks!
*I realize we may be getting away from the English string being the key soon.
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.
Ah! I might have missed something when looking at the diff then, as I was convinced that those were newly introduced strings — as opposed to already existing ones that we just moved around or reused in a different context.
If they were already part of the strings that got extracted in the generated Localizable.strings
back when Gio did the code freeze (maybe because they were already used / present in another part of the code) then we should be all good for those then!
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.
Cool!
"Create Site" is a "new" string to that view, but it was used (in the same context) in WPiOS prior to this change, so my hope is that the translation for that key is just reused.
"Choose" already existed on this view, it was only moved. 👍
"\n" | ||
"What’s in a name? Well, if your site has one, you’ll see it in the My Site navigation title.\n" | ||
"\n" | ||
"When you swipe left on a comment in your Notifications, you won’t see the “Trash” option anymore. Instead you’ll see “Unapprove Comment” and “Approve Comment.” We approve.\n" |
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 too feel this wording is safer than the more spicy 🌶️ one Grace propose 😅 (While it was funny, the alternative spicier copy might have been a lost reference that not everybody would have got, and could also have been negatively interpreted)
@@ -2169,6 +2169,7 @@ | |||
C373D6EA280452F6008F8C26 /* SiteIntentDataTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = C373D6E9280452F6008F8C26 /* SiteIntentDataTests.swift */; }; | |||
C387B7A22638D66F00BDEF86 /* PostAuthorSelectorViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = C3E2462826277B7700B99EA6 /* PostAuthorSelectorViewController.swift */; }; | |||
C38C5D8127F61D2C002F517E /* MenuItemTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = C38C5D8027F61D2C002F517E /* MenuItemTests.swift */; }; | |||
C396C80B280F2401006FE7AC /* SiteDesignTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = C396C80A280F2401006FE7AC /* SiteDesignTests.swift */; }; |
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.
Woohoo, more tests! 🎉 ❤️
@@ -1 +1 @@ | |||
Bir web sitesi/blog oluşturun | |||
Siteni tasarla, blog oluştur |
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.
Still <30, all good 👍 😌
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.
Approving now that my two previous
Includes:
Nothing to test, if CI Shows green, we're good.
Regression Notes
RELEASE-NOTES.txt
if necessary. N.A.