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 VideoPress block for useBlockProps #19913

Merged
merged 3 commits into from
May 24, 2021
Merged

Update VideoPress block for useBlockProps #19913

merged 3 commits into from
May 24, 2021

Conversation

jgcaruso
Copy link
Contributor

Fixes #18864

Changes proposed in this Pull Request:

  • extracts the video player portion of the rendered component into a function component so the useBlockProps hook can be called

Jetpack product discussion

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

Testing instructions:

  • Ensure you have a Jetpack site with VideoPress enabled
  • Prior to pulling PR, create a post and add a video block (it may be unclickable and very wide, this PR will fix that)
  • Apply the PR
  • Refresh the post and the videoblock should still work
  • Add a new video block, it should render as expected
  • Ensure all video block interactions continue to work as expected

@jgcaruso jgcaruso requested a review from roundhill May 21, 2021 17:58
@jgcaruso jgcaruso self-assigned this May 21, 2021
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello jgcaruso! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D61748-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 the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label May 21, 2021
@github-actions
Copy link
Contributor

github-actions bot commented May 21, 2021 •
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 🤖

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: June 1, 2021.
  • Scheduled code freeze: May 24, 2021.

@github-actions github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label May 21, 2021
@jgcaruso jgcaruso added [Status] Needs Team Review and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels May 21, 2021
@jgcaruso jgcaruso force-pushed the update/videopress-use-block-props branch from d6204ca to 3d12735 Compare May 21, 2021 19:45
roundhill
roundhill approved these changes May 21, 2021
View reviewed changes
Copy link
Contributor

@roundhill roundhill left a comment

Choose a reason for hiding this comment

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

Tested fine for me! Looks like we're safe from using deprecation because the props/markup don't change? Old VideoPress blocks still loaded up fine for me.

projects/plugins/jetpack/extensions/blocks/videopress/edit.js
</Fragment>
);
}
};

// The actual, final rendered video player markup
// In a separate function component so that `useBlockProps` could be called.
const VpBlock = props => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Totally not a blocker, but what if this was VideoBlock or VideoPressBlock, it just looks a little funny to my eyes.

@jeherve jeherve added this to the jetpack/9.8 milestone May 24, 2021
@roundhill roundhill added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Team Review labels May 24, 2021
@roundhill roundhill mentioned this pull request May 24, 2021
Closed
2 tasks
jeherve
jeherve approved these changes May 24, 2021
View reviewed changes
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.

This looks good to me. 👍

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! [Type] Janitorial and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels May 24, 2021
@roundhill
Copy link
Contributor

Thanks @jeherve, @jgcaruso is AFK today so shall I just merge this? Looks like today is the deadline?

@jeherve
Copy link
Member

jeherve commented May 24, 2021

Yep, merge away! 🥳

@roundhill roundhill merged commit 64171f5 into master May 24, 2021
@roundhill roundhill deleted the update/videopress-use-block-props branch May 24, 2021 17:32
@github-actions
Copy link
Contributor

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

Thank you!

@github-actions github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 24, 2021
@jgcaruso jgcaruso restored the update/videopress-use-block-props branch May 25, 2021 13:53
@jgcaruso
Copy link
Contributor Author

Reopened branch to get D61748-code to rebuild

@jgcaruso
Copy link
Contributor Author

committed with changeset id 226216

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

@roundhill roundhill roundhill approved these changes

@jeherve jeherve jeherve approved these changes

Assignees

@jgcaruso jgcaruso

Labels
[Block] VideoPress [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ Touches WP.com Files [Type] Janitorial
Projects
None yet
Milestone
jetpack/9.8
Development

Successfully merging this pull request may close these issues.

Blocks: switch away from __experimentalBlock to useBlockProps
4 participants
@jgcaruso @matticbot @roundhill @jeherve