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

Update react-native-video dependency to remove Apache 2.0 dependencies #2124

Merged
merged 2 commits into from
Apr 22, 2020

Conversation

ceyhun
Copy link
Contributor

@ceyhun ceyhun commented Apr 7, 2020

Fixes WordPress/gutenberg#19860

react-native-video PR: wordpress-mobile/react-native-video#6

To test:

  • Add a Video block check if behaviour stays the same
  • Same for Media & Text block with a video
  • Same for Cover block with a video

PR submission checklist:

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

@ceyhun ceyhun requested a review from cameronvoell April 7, 2020 17:53
@ceyhun ceyhun changed the title [Monorepo] Update react-native-video dependency to remove Apache 2.0 dependencies Update react-native-video dependency to remove Apache 2.0 dependencies Apr 9, 2020
Copy link
Contributor

@cameronvoell cameronvoell left a comment

Choose a reason for hiding this comment

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

This looks fine on iOS. On android, I don't think it is making anything worse than it already is.

However, on Android if I add a video block, a media + text block, and cover block in one article, and add one 818kb, 1 second mp4 video taken with my android phone (and stored in my WordPress media library), the previews do not seem to show up ever on Android. Sometimes they'll show up the first time I add the block and photo, but if I leave the editor and come back to the draft, the previews never load. I let my phone sit for 5 minutes plus, and the previews never load. I see them load immediately on iPhone every time. To be clear I see this problem on Android in the Play Store beta build, on gb-mobile current develop, and on this branch, so it is not unique to the change here.

@ceyhun Can you verify if leaving and coming back to an article draft with any video blocks on Android shows any preview for you? This could be out of scope, but then again, it's hard to tell if the update here is breaking anything if Android video blocks are just black screens in the editor before and after the change

@ceyhun
Copy link
Contributor Author

ceyhun commented Apr 17, 2020

@cameronvoell It works fine for me 😕
I tried on a Huawei P20 lite with Android 9 from release/14.6 branch of WordPress-Android while a metro server was running on this PR's branch.
I tried with SD version of this video on my phone https://www.pexels.com/video/wind-turbine-on-a-field-at-sunrise-857010/ with a Video block and with both Choose from device and WordPress Media Library.

@ceyhun
Copy link
Contributor Author

ceyhun commented Apr 21, 2020

@cameronvoell I was able to reproduce it just now on Android 14.5, Play Store version choosing WordPress Media Library with the same block, video and device. As you said, it works fine on iOS.

@ceyhun
Copy link
Contributor Author

ceyhun commented Apr 21, 2020

I was able to reproduce on iOS as well on 14.6, App Store version with choosing WordPress Media Library. But after closing, saving and re-opening the draft it worked.

I think it's this issue #1151 and I'll add a comment over there.

@cameronvoell
Copy link
Contributor

I think it's this issue #1151 and I'll add a comment over there.

Comment looks good over there, thanks for checking and documenting that. This change should be good in the mean time 👍 :shipit:

@ceyhun ceyhun merged commit 0482b3e into develop Apr 22, 2020
@ceyhun ceyhun deleted the issue/remove-apache2-dependencies branch April 22, 2020 13:53
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.

[Monorepo] Update react-native-video dependency to remove Apache 2.0 dependencies
3 participants