Skip to content

Conversation

@roundhill
Copy link
Contributor

We added support to the VideoPress player to support the playsinline attribute which will prevent a video from automatically playing full screen when on mobile. This PR adds a user-facing UI for that attribute to be set.

Screen Shot 2021-03-29 at 2 01 09 PM

Changes proposed in this Pull Request:

  • Adds a new toggle to the VideoPress block settings (disabled by default) that will allow a user to set if the video should be played inline in the browser.

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

No

Testing instructions:

  • Before applying PR, add a VideoPress block to a post with a video and save it.
  • Apply the PR and rebuild Jetpack. Load up the post you saved to ensure the block still loads properly. The Plays Inline setting should be untoggled.
  • Add another new post and add a video block. Turn on the Plays Inline toggle, and publish the post.
  • Inspect the video, and you should see the playsinline attribute applied to the video tag.
  • Bring up the url on a mobile device (I tested on iPhone 12). Pressing play should play the video inline on the page, not taking you full screen.

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello roundhill! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D59449-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@github-actions github-actions bot added [Block] VideoPress [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ labels Mar 29, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Mar 29, 2021

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 🤖


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.


Jetpack plugin:

  • Next scheduled release: May 4, 2021.
  • Scheduled code freeze: April 26, 2021.

@github-actions github-actions bot added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Mar 29, 2021
@roundhill roundhill added [Status] Needs Team Review Obsolete. Use Needs Review instead. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Mar 29, 2021
@github-actions github-actions bot added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Mar 29, 2021
Copy link
Contributor

@jgcaruso jgcaruso left a comment

Choose a reason for hiding this comment

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

Code looks good and testing on mobile worked as expected.

One thing I noticed, not a result of this PR but something we probably need to address in the player, tapping on the video doesn't pause it. You need to tap the pause button directly in order to pause the video.

@roundhill roundhill added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Team Review Obsolete. Use Needs Review instead. labels Mar 30, 2021
@jeherve jeherve added this to the jetpack/9.7 milestone Mar 31, 2021
@jeherve jeherve removed the [Status] Needs Review This PR is ready for review. label Mar 31, 2021
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

While the PR adds the attribute well, I have some questions:

  1. I don't see a difference in behaviour in Chrome on my Android device. Is that expected?
  2. "Play Inline" doesn't really tell us much about what the toggle will change in our video? Could we add some help text, like it's done for the Autoplay toggle?

@roundhill
Copy link
Contributor Author

  • I don't see a difference in behaviour in Chrome on my Android device. Is that expected?

Chrome doesn't go full-screen by default, so yeah that's expected. I think this is mainly for iOS.

  • "Play Inline" doesn't really tell us much about what the toggle will change in our video? Could we add some help text, like it's done for the Autoplay toggle?

Nice idea but it kinda sticks out like a sore thumb:

Screen Shot 2021-03-31 at 8 56 07 AM

It'd be nice if the ToggleControl could do a hover tooltip but I didn't see a way to add that?

@jeherve
Copy link
Member

jeherve commented Mar 31, 2021

Nice idea but it kinda sticks out like a sore thumb:

I didn't find this too disturbing since it only appears when the toggle is on, and since that's already how the Autoplay toggle works:

image

Maybe that's just me though ¯_(ツ)_/¯

I'll let someone else weigh in on this. :)

@jeherve jeherve added the [Status] Needs Design Review Design has been added. Needs a review! label Mar 31, 2021
@jeherve
Copy link
Member

jeherve commented Apr 6, 2021

Internal reference: r223581-wpcom

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Since this is already committed to WordPress.com, let's keep the codebases in sync. We can see if we get feedback from post authors about what the new setting means.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. [Status] Needs Design Review Design has been added. Needs a review! labels Apr 6, 2021
@roundhill roundhill merged commit 6bc2a4b into master Apr 13, 2021
@roundhill roundhill deleted the add/videopress-block-playsinline branch April 13, 2021 18:12
@github-actions github-actions bot removed [Status] Design Input Requested [Status] Ready to Merge Go ahead, you can push that green button! labels Apr 13, 2021
@github-actions
Copy link
Contributor

Great news! One last step: head over to your WordPress.com diff, D59449-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Block] VideoPress [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Status] Tested on WP.com Touches WP.com Files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants