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

[RNMobile] VideoPress Android WebView #30521

Merged
merged 15 commits into from
May 11, 2023
Merged

[RNMobile] VideoPress Android WebView #30521

merged 15 commits into from
May 11, 2023

Conversation

jhnstn
Copy link
Member

@jhnstn jhnstn commented May 8, 2023

Fixes wordpress-mobile/gutenberg-mobile#5509

Depends on:

Proposed changes:

  • Integrate the new embed preview WebView on Android

Other information:

  • Have you written new tests for your changes, if applicable?
  • Have you checked the E2E test CI results, and verified that your changes do not break them?
  • Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

See Test Instructions in the Gutenberg PR

@jhnstn jhnstn added the [Feature] VideoPress A feature to help you upload and insert videos on your site. label May 8, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 8, 2023 •
edited
Loading

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

@github-actions
Copy link
Contributor

github-actions bot commented May 8, 2023 •
edited
Loading

Are you an Automattician? You can now test your Pull Request on WordPress.com. On your sandbox, run

bin/jetpack-downloader test jetpack videopress/rnmobile/android-webview

to get started. More details: p9dueE-5Nn-p2

@jhnstn jhnstn mentioned this pull request May 8, 2023
Merged
2 tasks
@jhnstn jhnstn mentioned this pull request May 9, 2023
Merged
12 tasks
@jhnstn jhnstn requested review from fluiddot and SiobhyB May 9, 2023 03:18
@jhnstn jhnstn marked this pull request as ready for review May 9, 2023 03:18
@jhnstn jhnstn changed the title Videopress/rnmobile/android webview [RNMobile] VideoPress Android WebView May 9, 2023
@jhnstn jhnstn marked this pull request as draft May 9, 2023 03:48
@jhnstn
Copy link
Member Author

jhnstn commented May 9, 2023

Keeping as a draft till #30411 is merged

@jhnstn jhnstn changed the base branch from trunk to rnmobile/update/loading-screen-android May 9, 2023 03:49
Base automatically changed from rnmobile/update/loading-screen-android to trunk May 9, 2023 07:30
fluiddot
fluiddot reviewed May 9, 2023
View reviewed changes
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.

@jhnstn Tested via wordpress-mobile/WordPress-Android#18383 (review). I added a couple of comments that shouldn't block the PR, let me know if you could take a look, thanks!

...s/packages/videopress/src/client/block-editor/blocks/video/components/player/index.native.js Outdated Show resolved Hide resolved
projects/packages/videopress/changelog/rnmobile-update-loading-screen-android Outdated Show resolved Hide resolved
...s/packages/videopress/src/client/block-editor/blocks/video/components/player/index.native.js Outdated Show resolved Hide resolved
@jhnstn jhnstn marked this pull request as ready for review May 9, 2023 17:53
@jhnstn jhnstn requested a review from fluiddot May 9, 2023 17:54
@jhnstn jhnstn mentioned this pull request May 9, 2023
Open
fluiddot
fluiddot reviewed May 10, 2023
View reviewed changes
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.

Tested via wordpress-mobile/WordPress-Android#18383 (review).

...s/packages/videopress/src/client/block-editor/blocks/video/components/player/index.native.js Outdated Show resolved Hide resolved
...packages/videopress/src/client/block-editor/blocks/video/components/player/style.native.scss Outdated Show resolved Hide resolved
@jhnstn jhnstn mentioned this pull request May 10, 2023
Merged
@jhnstn jhnstn requested a review from fluiddot May 10, 2023 16:04
fluiddot
fluiddot reviewed May 10, 2023
View reviewed changes
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.

@jhnstn looks like Linting / ESLint jobs are failing.

...s/packages/videopress/src/client/block-editor/blocks/video/components/player/index.native.js Outdated Show resolved Hide resolved
@jhnstn jhnstn requested a review from fluiddot May 10, 2023 18:01
@jhnstn jhnstn enabled auto-merge (squash) May 10, 2023 21:43
fluiddot
fluiddot reviewed May 11, 2023
View reviewed changes
...s/packages/videopress/src/client/block-editor/blocks/video/components/player/index.native.js
Comment on lines +155 to +160
<Pressable
style={ style[ 'videopress-player__open-embed-button' ] }
onPress={ () => {
requestEmbedFullscreenPreview( html, title );
} }
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Following some of the issues we spotted when testing (wordpress-mobile/gutenberg-mobile#5758), I'm wondering if we should limit opening the WebView until the video reaches a ready state. Maybe we could wait until the player is ready and loaded to enable this button, WDYT?

I was also thinking if we could get the HTML from the WebView we display within the editor to pass it to the new WebView, but seems this data is not exposed by the react-native-webview library.

In any case, since the companion PRs to this one have been merged, I'll go ahead and merge it as-is. Hence, potential improvements to this approach would need a separate PR.

fluiddot
fluiddot approved these changes May 11, 2023
View reviewed changes
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.

Approved via wordpress-mobile/WordPress-Android#18383 (review).

@jhnstn jhnstn merged commit 12e194b into trunk May 11, 2023
@jhnstn jhnstn deleted the videopress/rnmobile/android-webview branch May 11, 2023 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fluiddot fluiddot fluiddot approved these changes

@SiobhyB SiobhyB Awaiting requested review from SiobhyB

Assignees
No one assigned
Labels
[Feature] VideoPress A feature to help you upload and insert videos on your site. [Package] VideoPress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Video player
2 participants
@jhnstn @fluiddot