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

Audio block capability now enables/disables media upload's media sources #3523

Merged
merged 19 commits into from
Jun 3, 2021

Conversation

jd-alexander
Copy link
Contributor

@jd-alexander jd-alexander commented May 19, 2021

Fixes #3434

WordPress iOS wordpress-mobile/WordPress-iOS#16533
WordPress Android wordpress-mobile/WordPress-Android#14680
Gutenberg WordPress/gutenberg#31966

To test:
WordPress/gutenberg#31966

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 19, 2021

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

@jd-alexander
Copy link
Contributor Author

@fluiddot I am going to be finalizing these set of PRs soon with the capabilities changes. While working on this on Friday, I noticed the translation related failure while trying to build the bundle to test on iOS. I am assuming that I have to utilize the string-extract-fix patch and localization files from 1.52.2 you created for the previous release to get this working as expected. Let me know what you think.

@fluiddot
Copy link
Contributor

@fluiddot I am going to be finalizing these set of PRs soon with the capabilities changes. While working on this on Friday, I noticed the translation related failure while trying to build the bundle to test on iOS. I am assuming that I have to utilize the string-extract-fix patch and localization files from 1.52.2 you created for the previous release to get this working as expected. Let me know what you think.

Yeah, I'd recommend you to use the patch and the localization files from 1.52.2 to produce the proper localization strings files. However, if you're not introducing new strings, probably you could skip modifying those files in this PR and use the ones we currently have on develop branch.

Btw, I've just checked the commit "Merge branch 'develop' into add/audio-block-capability" and I saw that some of the strings have been pushed with conflicts.

@jd-alexander
Copy link
Contributor Author

jd-alexander commented May 25, 2021

Yeah, I'd recommend you to use the patch and the localization files from 1.52.2 to produce the proper localization strings files. However, if you're not introducing new strings, probably you could skip modifying those files in this PR and use the ones we currently have on develop branch.

Thanks for the tips. I haven't updated this PR as yet as I was investigating the hermes crash which is a blocker for this PR since I would need to sync with the latest gb-mobile develop and gb trunk.

@jd-alexander jd-alexander force-pushed the add/audio-block-capability branch from 6a0e564 to f314419 Compare May 27, 2021 01:26
@jd-alexander
Copy link
Contributor Author

jd-alexander commented May 27, 2021

Since there are changes within the main apps PR apart from the gutenberg-mobile ref changes, I will be creating a gutenberg-mobile tag of v1.54.0-alpha2 when this PR is approved.

@fluiddot when I run npm run bundle with or without the patch I was getting genstring failures so I am not sure if the translation changes here aren't to be expected. Let me know. Thanks! I am going to give the PR you created and related discussions a thorough review to understand how translations work because I haven't had a look at the code generation logic in detail.

@jd-alexander
Copy link
Contributor Author

I did re-runs of Gutenberg Editor Slash Inserter tests but I am still getting failures. The changes here are unrelated as far as I am concerned.

@fluiddot
Copy link
Contributor

Since there are changes within the main apps PR apart from the gutenberg-mobile ref changes, I will be creating a gutenberg-mobile tag of v1.54.0-alpha2 when this PR is approved.

👍

@fluiddot when I run npm run bundle with or without the patch I was getting genstring failures so I am not sure if the translation changes here aren't to be expected. Let me know. Thanks! I am going to give the PR you created and related discussions a thorough review to understand how translations work because I haven't had a look at the code generation logic in detail.

Yikes, could you share the failures that you got when generating the bundle?

I see that these changes have a new string (Insert from URL) in the code but I verified that it's already in the Gutenberg translations (GlotPress reference) so it's expected that no modification is made to the localization strings files that we push to the main apps.

  • bundle/android/strings.xml
  • bundle/ios/GutenbergNativeTranslations.swift

However, I saw the translation files (i18n-data files) have been modified and they are missing strings (you can check that by opening any of them (es.json) and look for the word block title). I'll apply the fixes from the PR that should help us with this issue.

@fluiddot
Copy link
Contributor

I did re-runs of Gutenberg Editor Slash Inserter tests but I am still getting failures. The changes here are unrelated as far as I am concerned.

This was fixed yesterday via this PR so if you update the branch with develop it should be fixed 🎊 .

fluiddot added 3 commits May 31, 2021 18:51
# Conflicts:
#	bundle/ios/App.js
#	bundle/ios/App.js.map
#	gutenberg
@fluiddot
Copy link
Contributor

@jd-alexander Heads up that I've updated all the PRs for doing final testing, the only changes I made are:

Let me know if you have any concerns regarding these updates, thanks 🙇 !

@jd-alexander
Copy link
Contributor Author

Let me know if you have any concerns regarding these updates, thanks 🙇 !

I have no issue with these updates. Thanks @fluiddot 🙇🏾

@@ -1,3 +1,7 @@
Unreleased
------
* [***] Audio block now available on WP.com sites on the free plan. [https://github.com/wordpress-mobile/gutenberg-mobile/pull/3523]
Copy link
Contributor

Choose a reason for hiding this comment

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

I moved this entry to the unreleased section as this PR won't be included in 1.54.0.

@fluiddot fluiddot modified the milestones: 1.54.0 (17.5), 1.55.0 (17.6) Jun 1, 2021
@fluiddot
Copy link
Contributor

fluiddot commented Jun 1, 2021

I changed the milestone of this PR to 1.55.0 (17.6) as the 1.54.0 version is already cut.

Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

@fluiddot fluiddot self-assigned this Jun 3, 2021
@fluiddot fluiddot merged commit 10eb321 into develop Jun 3, 2021
@fluiddot fluiddot deleted the add/audio-block-capability branch June 3, 2021 13:24
@antonis antonis mentioned this pull request Jun 11, 2021
5 tasks
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.

[Audio block] Disable media upload options on free sites
2 participants