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.

Sign up for GitHub

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

Jump to bottom

Update video player to open the URL by browser on Android #1117

Merged
merged 8 commits into from
Jun 14, 2019
Merged

Update video player to open the URL by browser on Android #1117

merged 8 commits into from
Jun 14, 2019

Conversation

pinarol
Copy link
Contributor

@pinarol pinarol commented Jun 11, 2019 •
edited
Loading

Fixes #1119

gutenberg PR: WordPress/gutenberg#16089

This should change only Android. We are canceling out the react-native-video's player controls and replacing it with system browser:

video-android

iOS should remain same:

video-ios

To test:

WPAndroid:

  • Open a post with video
  • Verify that videos have a single play button and controls are invisible
  • Tap on the video
  • Verify that video opens outside of the app, probably in browser(I think it might also be opened by another app if user has selected it as the default app to open videos)

WPiOS should remain same:

  • Open a post with video
  • Tap on the video
  • Verify that video opens fullscreen INSIDE the app

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@pinarol pinarol added the Media label Jun 11, 2019
@pinarol pinarol added this to the v1.7 milestone Jun 11, 2019
@pinarol pinarol self-assigned this Jun 11, 2019
@pinarol pinarol mentioned this pull request Jun 11, 2019
Merged
5 tasks
@hypest hypest requested a review from marecar3 June 11, 2019 15:46
@hypest
Copy link
Contributor

hypest commented Jun 11, 2019

I can confirm that this makes the flow better, at least for the time being until the inline previewing works. I haven't given a look at the code side of things though so, I'm just adding this comment without aprooving the PR.

@hypest hypest mentioned this pull request Jun 12, 2019
Closed
5 tasks
@hypest
Copy link
Contributor

hypest commented Jun 13, 2019

We decided to hide the video block on Android for the 1.7 release (#1127) so, I'll remove this PR from the milestone and re-target it towards develop. Let's decide later if we want to work some more on this one or note.

@hypest hypest removed this from the v1.7 milestone Jun 13, 2019
@hypest hypest changed the base branch from release/1.7.0 to develop June 13, 2019 11:58
@pinarol
Copy link
Contributor Author

pinarol commented Jun 13, 2019

I think this PR brings a nice enhancement and worth merging it even if it doesn't solve the original overflow problem. Because the browser has a much better player. But even if we don't merge this, let's not forget to add the bottom item on a separate PR:

  • Showing the play button even if video is not ready to play(this will be valid only for only iOS in case of discarding this PR)

@daniloercoli
Copy link
Contributor

daniloercoli commented Jun 14, 2019 •
edited
Loading

I'm ok on merging this one, since external player(s) (apps or browser) are way better. Considering also the block has limited dimension, and the play'ed video inside them has a small viewport.

About the overflow problem, I suggest to wait until the AndroidX migration is completed.

If we agree to merge this PR we need upgrade the GB side companion PR to bring in latest changes.

@daniloercoli
Merge branch 'develop' of https://github.com/wordpress-mobile/gutenbe…
db52115 
…rg-mobile into issue/1037-video-layout-problem-on-android

* 'develop' of https://github.com/wordpress-mobile/gutenberg-mobile: (42 commits)
  Point to merged mobile release on GB master
  Update JS bundles
  Update GB ref
  Point to the merged-to-release GB hash
  Update bundles.
  Let RichText compute shouldBlurOnUnmount by its own
  Rename to an unstable, very goal-named name
  Update gutenberg reference
  Declare isReplaceable's type
  Update gutenberg reference
  Point to a branched GB hash
  Pass isReplaceable via Context so RichText can blur
  Disable Video block on Android for v1.7.0 release
  Remove commented out waits in UI test utils
  Update gutenberg ref
  Update gutenberg ref
  Move BlockPicker to gutenberg as Inserter in @wordpress/block-editor
  Update gutenberg ref
  typing out long text taking too long, reduing amount text needed to type for tests using long text
  fix submodule
  ...

# Conflicts:
#	gutenberg
@daniloercoli
Update GB ref.
286f23d
daniloercoli
daniloercoli approved these changes Jun 14, 2019
View reviewed changes
Copy link
Contributor

@daniloercoli daniloercoli left a comment

Choose a reason for hiding this comment

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

So let's merge this one once GB side is green.

We will take care of fixing video problems on Android on another PRs, maybe we need to wait until AndroidX lands into the house, but we will see if it's a blocker or not. Keep the video block unregistered for now.

@pinarol pinarol merged commit fc362da into develop Jun 14, 2019
@pinarol pinarol deleted the issue/1037-video-layout-problem-on-android branch June 14, 2019 15:13
@designsimply designsimply mentioned this pull request Oct 24, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daniloercoli daniloercoli daniloercoli approved these changes

@hypest hypest Awaiting requested review from hypest

@koke koke Awaiting requested review from koke

@marecar3 marecar3 Awaiting requested review from marecar3

Assignees

@pinarol pinarol

Labels
Media
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Android]Video player overflows out of its bounds after playing is started
3 participants
@pinarol @hypest @daniloercoli