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

Merge fixes coming from 19.4 beta 2 #18136

Merged
merged 11 commits into from
Mar 15, 2022
Merged

Merge fixes coming from 19.4 beta 2 #18136

merged 11 commits into from
Mar 15, 2022

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Mar 15, 2022

Includes:

  • Fix the ghost view not dismissing from the Insight tab under certain conditions (#18104) by @Gio2018
  • Fix an issue preventing users from changing their profile picture (#18132) by @guarani

Note that some of the translations are "incorrect". I've either addressed them in GlotPress or will do that "soon". If I don't get to it before you read this, I'd say we should merge the PR anyways. Otherwise, I'll run a new cycle of GlotPress downloads and all my notes about incorrect translations should disappear.

I'm keen on merging this even though the translations could be fixed in the context of this PR because:

  • I want to avoid conflicts with trunk
  • The sooner we get the beta fixes in trunk the better
  • There's no risk of forgetting about the translation issues because our automation will re-download them next time. Everything that's fixed will be fixed and if something hasn't been yet, we'll notice it in the diff.

@peril-wordpress-mobile
Copy link

Messages
📖 This PR has the 'Releases' label: some checks will be skipped.

Generated by 🚫 dangerJS

@mokagio
Copy link
Contributor Author

mokagio commented Mar 15, 2022

🤔 we got a UI tests failure on iPhone. By looking at the attached .xcresult, one can see the test crashed:

UI Test Activity:
Crash: backboardd (1773) 0x10726b000: Namespace SIGNAL, Code 0xb. dyld4 config: DYLD_ROOT_PATH=/Applications/Xcode-13.app/Contents/Developer/Platforms/iPhoneOS.platform/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS.simruntime/Contents/Resources/RuntimeRoot
CoreSimulator 776.3 - Device: iPhone 13 (79FE6B87-502C-4233-8A3B-70C5B688E4BE) - Runtime: iOS 15.0 (19A339) - DeviceType: iPhone 13

image

I restarted the tests.

Copy link
Contributor Author

@mokagio mokagio left a 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

@@ -12,6 +12,7 @@
* [*] Weekly Roundup: We made some further changes to try and ensure that Weekly Roundup notifications are showing up for everybody who's enabled them [#18029]
* [*] Block editor: Autocorrected Headings no longer apply bold formatting if they weren't already bold. [#17844]
* [***] Block editor: Support for multiple color palettes [https://github.com/wordpress-mobile/gutenberg-mobile/pull/4588]
* [**] User profiles: Fixed issue where the app wasn't displaying any of the device photos which the user had granted the app access to.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was done knowingly of the fact the new entry wouldn't have made it in the editorialized copy: #18132 (comment)

@@ -1 +1 @@
WordPress
워드프레스 – 웹사이트 제작 도구
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated this at the GlotPress source.

Before:

image

After:

image

Concevez un site, créez un blog
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is 31 characters, where it should be 30.

I don't know how to fix this I'll have to ask the i18n team.

@@ -1 +1 @@
WordPress
ووردبريس – مُنشئ مواقع الويب
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I "fixed" this at the GlotPress source. I'm not actually sure the way I wrote it is correct, but GlotPress marks my changed as waiting, so someone will look at it soon, hopefully.

Before:

image

After:

image

مدون، كتابة، تدوين، ويب، صانع، عبر الإنترنت، متجر، أعمال، إعداد، إنشاء، كتابة، مدونات
Copy link
Contributor Author

@mokagio mokagio Mar 15, 2022

Choose a reason for hiding this comment

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

This is less than the required 100 characters ✅

But I'm not sure about "jetpack" having been translated. I think the same rationale applies as WordPress...

I'll fix it at the source later, too.

Update: That "jetpack" keyword made me think these are the Jetpack app keywords, but they're the WordPress ones, as it's obvious once you look at the path 🤦

The new keywords do not include Jetpack. I run these through Google Translate and they match 👍 .

There is only one thing that's off and that's the use of Arabic commas instead of English one.

I updated that in the GlotPress source:

image

@mokagio mokagio marked this pull request as ready for review March 15, 2022 06:03
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 15, 2022

You can test the Jetpack changes on this Pull Request by downloading it from AppCenter here with build number: pr18136-94fa338. IPA is available here. If you need access to this, you can ask a maintainer to add you.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 15, 2022

You can test the WordPress changes on this Pull Request by downloading it from AppCenter here with build number: pr18136-94fa338. IPA is available here. If you need access to this, you can ask a maintainer to add you.

@mokagio mokagio added this to the 19.4 ❄️ milestone Mar 15, 2022
@mokagio mokagio requested a review from a team March 15, 2022 06:05
Copy link
Contributor

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

  • Changes from bugfix PRs which landed in the release branch
  • Version bump in .xcconfig files
  • Latest app and metadata translations pulled from GlotPress

I'm all good with merging the even if some of the metadata translations will have to be fixed later — especially since they are about store metadata (and not in-app translations in .strings files that would be visible to beta-testers).

As long as you took note that you'll have to loop back with i18n and polyglots to help fix them before you submit to the store on Friday, I don't see any harm in having them land in trunk for the time being. 👍


💭 PS: We could actually even argue if it really is useful to download store metadata during new_beta_release — well given current legacy tooling we can't really separate the two, but it's worth noting that with the new actions it is now possible so could be a thought to keep in mind, i.e. only download in-app translations during new_beta_release and only download both in-app and metadata translations during finalize_release 🤔 🤷

@mokagio mokagio merged commit 4def945 into trunk Mar 15, 2022
@mokagio
Copy link
Contributor Author

mokagio commented Mar 15, 2022

💭 PS: We could actually even argue if it really is useful to download store metadata during new_beta_release — well given current legacy tooling we can't really separate the two, but it's worth noting that with the new actions it is now possible so could be a thought to keep in mind, i.e. only download in-app translations during new_beta_release and only download both in-app and metadata translations during finalize_release 🤔 🤷

That intriguing 🤔 I like the idea of not doing unnecessary work and to keep the beta merges diffs clean. At the same time, beta downloads allow us to discover issues early, like this PR shows. 🤔 🤔 🤔

Oh, and thank you for the review 🙇‍♂️

Ontwerp een site, bouw een blog
Copy link
Contributor Author

Choose a reason for hiding this comment

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

31 characters.

Designa en webbplats, bygg en blogg
Copy link
Contributor Author

Choose a reason for hiding this comment

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

35 characters

@@ -1 +1 @@
WordPress
WordPress – Web Sitesi Oluşturucu
Copy link
Contributor Author

Choose a reason for hiding this comment

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

33 characters

Bir site tasarla, bir blog oluştur
Copy link
Contributor Author

Choose a reason for hiding this comment

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

34 characters

@AliSoftware
Copy link
Contributor

At the same time, beta downloads allow us to discover issues early, like this PR shows.

Good point! Let's keep it then, it's not like the metadata download takes that much time anyway.

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.

6 participants