-
Notifications
You must be signed in to change notification settings - Fork 58
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
[Embed block] Include Jetpack embed variants #4008
Conversation
Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job! |
I did a first pass of the code and details within these PRs @fluiddot to acclimatize myself with the changes. I will do a thorough test of all the things tomorrow. Thanks for the clarity in breaking down how it all comes together 🙇🏾 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work @fluidot since these changes are so extensive I want to share the verification methods I did at each step for clarity.
Overview
Split setup and block registration
- I observed how the refactoring done in
jetpack-editor-setup.js
allowed you to leverage thenative.pre-render
andnative.render
hooks to order the Jetpack initialization code. - For the related Jetpack changes, I observed that the Facebook and Instagram variations are reactivated since core has no plans to support the oEmbed changes implemented by Facebook. However, Jetpack and WP.com supports this by sending the access token alongside the oEmbed API requests. I was able to follow how we brought these changes from Jetpack ➡️ Gutenberg-Mobile ➡️ Gutenberg ➡️ By leveraging the amazing power of hooks to enable the variations on the fly.
Remove timeout for hiding blocks
- I applaud you for optimizing the Jetpack setup logic with the ideation and implementation that resulted in
Remove timeout for hiding blocks
that leveraged the existing access we have to the editor capabilities 🎊
General Testing
Setup
I prepared my local environment for these tests by:
- modifying the
ALLOWED_EMBED_VARIATIONS
array so that the Jetpack embed variants aren't filter out of the inserter. - I utilized Jurassic.ninja to spin up a test site. I then connected/disconnected Jetpack.
- I utilized existing WP.com / atomic sites
Jetpack embed variants - Slash inserter ✅
I observed that the following terms were shown via the Slash inserter on iOS and Android:
- Loom
- Smartframe
Jetpack embed variants - Inserter menu ✅
I observed that the following terms were shown via the Inserter Menu on iOS and Android:
- Loom
- Smartframe
Potential regressions
I agree that the changes here refactored setup logic for previously integrated Jetpack blocks, so checks for regression are warranted.
WP.com site - Slash inserter ✅
I observed that the following terms were shown via the Slash inserter on iOS and Android along with the previously listed variants:
- Contact Info
- Story
WP.com site - Inserter menu ✅
I observed that the following terms were shown via the Inserter Menu on iOS and Android along with the previously listed variants:
- Contact Info
- Story
Atomic site - Slash inserter ✅
I observed that the following terms were shown via the Slash inserter on iOS and Android along with the previously listed variants:
- Contact Info
- Story
Atomic site - Inserter menu ✅
I observed that the following terms were shown via the Inserter Menu on iOS and Android along with the previously listed variants:
- Contact Info
- Story
Self-hosted site with Jetpack - Slash inserter ✅
I observed that the following terms were shown via the Slash inserter on iOS and Android along with the previously listed variants:
- Contact Info
- Story
- except Smartframe that will be released in 10.2
Self-hosted site with Jetpack - Inserter menu ✅
I observed that the following terms were shown via the Inserter Menu on iOS and Android along with the previously listed variants:
- Contact Info
- Story
- except Smartframe that will be released in 10.2
Self-hosted site without Jetpack - Slash inserter ✅
I observed that none of the Jetpack blocks were shown in the Slash inserter since Jetpack is not enabled.
Self-hosted site without Jetpack - Inserter menu ✅
I observed that none of the Jetpack blocks were shown in the Inserter since Jetpack is not enabled.
Since the behavior across all repos works as expected. We can get this moving. Enjoy the merge domino effect 🚢
# Conflicts: # bundle/ios/App.js # bundle/ios/App.js.map # jetpack
I'm afraid I forgot to add an entry into the release notes before merging, for this reason, I opened this PR to address this. |
Fixes #3951
Description
The following Jetpack embed variants are added (reference):
NOTE: Only the Instagram embed variant is meant to appear in the block inserter, as it's one of the most-used embeds (reference). The rest are expected to be found via the slash inserter.
To include these variants, I had to make a refactor of the Jetpack editor setup, in the following sections I describe the main changes:
Split setup and block registration
Jetpack embed variants use WordPress hooks that are attached to the block type registration (reference), so it's required to add them before the core blocks are registered. Since the Jetpack blocks are currently being registered after the core blocks are registered (triggered by
native.render
WordPress hook), I needed to separate the setup code so it can be called before the editor is initialized by the WordPress hooknative.pre-render
and the Jetpack embed variants are added.Remove timeout for hiding blocks
Jetpack blocks can be hidden by flags we include in the capabilities object passed from the main apps. I noticed that the capabilities are fetched from the block editor store via the
getSettings
selector. However, the capabilities are already included in the initial props we receive in the WordPress hooknative.pre-render
(reference that handles the logic for hiding the blocks, so I decided to refactor this part to remove the need of a timeout.When applying the refactor, I thought that maybe this approach was already considered back then but a blocker was found, so I went through the commit history of the
jetpack-editor-setup.js
file just in case, but I haven't found any note or caveat:Additional information
This PR also bumps the Gutenberg reference to include the following PR which is required for rendering the Jetpack embed variants:
WordPress/gutenberg#35053
To test
Preparation:
For now, the Jetpack embed variants are filtered out from the inserter menu so for some of the tests, it's required to make first the following manual code changes:
ALLOWED_EMBED_VARIATIONS
array located in this line:Jetpack embed variants - Slash inserter
Jetpack embed variants - Inserter menu
Potential regressions
Since this PR refactored the setup of Jetpack blocks, it would be great to double-check that it doesn't introduce regressions.
WP.com site
Atomic site
Self-hosted site with Jetpack
NOTE: Smartframe variation will be released on Jetpack
10.2
(scheduled on October 5, 2021), so it won't available on self-hosted sites connected with Jetpack until that date.Self-hosted site without Jetpack
PR submission checklist: