-
Notifications
You must be signed in to change notification settings - Fork 826
feat: Expand editor assets endpoint allowed block types #44979
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
base: trunk
Are you sure you want to change the base?
feat: Expand editor assets endpoint allowed block types #44979
Conversation
The styles are entirely absent for REST API requests unless they are universally registered.
Include additional block types found on WordPress.com.
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
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 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 1 file.
|
if ( is_admin() ) { | ||
add_action( 'enqueue_block_assets', array( Contact_Form_Block::class, 'load_editor_styles' ), 9 ); | ||
} |
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.
@coder-karen my understanding is that utilizing is_admin()
is the latest recommendation for enqueueing block editor content assets intended for the editor only (excluded from the site's front end). Unfortunately, the approach is problematic for REST API endpoints (like the block editor assets endpoint used for the mobile apps), as they are not "admin," which leads to the absence of this asset entirely—it's neither registered or enqueued.
To be clear, removing this is_admin()
check does results in loading an additional stylesheet for the site's front end:

I recognize this is likely undesired. I do not plan to merge this as-is currently.
My questions:
- What, specifically, are we avoiding with scoping this asset for the admin? Are the styles available/duplicated elsewhere? Are they truly only relevant to the admin? If so, how?
- Would the
is_admin()
usage cease and the REST API issue become moot when VULCAN-68 is completed? - Can you think of any low-effort workarounds to support the REST API but avoid duplicative style loads for the web?
Thank you for your help! 🙇🏻♂️
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.
To be clear, removing this is_admin() check does results in loading an additional stylesheet for the site's front end
Then that's a no-go, we shouldn't load more bytes in the frontend.
Have you looked at adding "is rest request" check? Looks like there's a helper:
jetpack/projects/packages/assets/src/class-script-data.php
Lines 107 to 114 in 4ec0394
/** | |
* Whether the current request is an authenticated REST API request. | |
* | |
* @return bool | |
*/ | |
protected static function is_authenticated_rest_request() { | |
return wp_is_serving_rest_request() && current_user_can( 'read' ); | |
} |
There's also REST_REQUEST
constant, some usage example here as alternative:
jetpack/projects/plugins/jetpack/extensions/blocks/subscriptions/subscriptions.php
Lines 712 to 719 in 4ec0394
// Only render the email version in non-frontend contexts. | |
if ( is_feed() || wp_is_xml_request() || | |
( defined( 'REST_REQUEST' ) && REST_REQUEST && ! wp_is_json_request() ) || | |
( defined( 'REST_API_REQUEST' ) && REST_API_REQUEST ) || | |
( defined( 'WP_CLI' ) && WP_CLI ) || | |
wp_is_jsonp_request() ) { | |
return render_for_email( $data, $styles ); | |
} |
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.
@simison thank you for providing feedback.
I agree. If the stylesheet in question should not be loaded in the site's front end, then we should not load the unnecessary bytes. My questions largely sought to under why the stylesheet should not be loaded (i.e., is it duplicating styles provided elsewhere?) and if there might be alternative style organization that would negate the need to avoid loading it.
Have you looked at adding "is rest request" check? Looks like there's a helper:
Yes, I actually introduced the referenced helper to the Jetpack codebase in 21ec749.
There's also
REST_REQUEST
constant, some usage example here as alternative:
The aforementioned helper and the utilities provided by WordPress core rely upon the REST_REQUEST
. Unfortunately, as mentioned in #44979 (comment), I believe the REST_REQUEST
constant is defined too late for the particular problem discussed in this thread. The constant is set after Jetpack decides whether to enqueue the targeted stylesheet.
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.
Yes, there's some CSS that specifically addresses editor only in the file. Much of it could be only in a separate view CSS file, but there's a lot of legacy here and much of the frontend styles are in grunion.scss.
Because some of the styles are needed just in editor, moving shared CSS around now wouldn't help though?
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.
Thank you for sharing the context regarding the editor stylesheet and accompanying grunion stylesheet.
Because some of the styles are needed just in editor, moving shared CSS around now wouldn't help though?
It's unclear to me currently if there is a possibility of relocating the missing styles to an always-loaded shared stylesheet. We'd need to dive a bit deeper into the specific missing styles to understand if they are truly editor-only or could be relocated to a shared place.
For example, elements like form-step
and form-progress-indicator
are un-styled in the mobile app editor due to the missing stylesheet.
Web | GutenbergKit |
---|---|
![]() |
![]() |
Presumably, those styles—form-step
and form-progress-indicator
—are necessary for both the admin editor and the site front end. For the editor, it appears those disparate styles are ultimately bundled with contact-form/editor.scss
and that is the currently admin-only stylesheet; on the site front end, those styles appear to be loaded as an individual stylesheets—e.g., jetpack-forms/dist/blocks/form-progress-indicator/style.css
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.
@lezama was looking a bit at organising our styles better for new fields, so I wonder if he would have thoughts here on short-term improvements?
Hi @dcalhoun I would say that generally with the changes you've mentioned, the REST API hasn't been considered. The forms change was only considering the editor, and the While I'm not too familiar with the current Contact Form scripts -- there are front-end scripts as well -- but I couldn't answer as to whether the editor script would cause any conflicts with those as the intention was to just keep the editor scripts in the editor. I don't think there is a definitive best practice here at the moment - I also wouldn't wait for VULCAN-68 to be completed either as team responsibilities shift (from the Vulcan pov), however I think it's important we note down why this is problematic so that REST API endpoints are not affected in the future(I've added a note in the issue). As for low-effort workarounds, while I'm not too familiar with supporting the REST API in this scenario there are a couple of new core functions that could be useful as well: wp_is_rest_endpoint and wp_is_serving_rest_request. I think any implementation would depend on what exactly is needed though. Are we talking about standalone REST API requests, or an internal request, for example. I would like to dig in more and also help to see what could work best for your use case, but am tight on time with upcoming AFK. That said, if you have more questions after today do feel free to reach out to Vulcan on Slack and the team can help look into this more if needed to. In terms of next steps, I think perhaps the best option then is not to look for a monorepo-wide approach (as I don't believe there is a best practice being followed here as such) but start with this one individually, and to check in with the forms team (#jetpack-forms) in terms of any risks associated with duplicating scripts for forms (if needed) - or just in terms of checking that any changes don't impact forms generally. |
Thank you for the guidance, @coder-karen.
Makes complete sense. Retrieving assets within the REST API is fairly novel, it's not surprising that implications on that are not on the forefront of most people's minds.
Thank you for the context regarding VULCAN-68's low priority. I am subscribed to it and will follow that for long-term plans.
I did explore those functions. Unfortunately, the context for those REST API functions is set after the point at which Jetpack decides whether or not to enqueue its stylesheet.
The block editor endpoint is to be used by the mobile apps. So, I do not believe it is considered an internal request.
I was unaware there was a dedicated Jetpack Forms team. I will follow up with them on this specific topic. Thank you! |
Fix CMM-660. Fix CMM-661.
Expand the allowed block types for the GutenbergKit mobile app editor.
Proposed changes:
Other information:
Jetpack product discussion
N/A
Does this pull request change what data or activity we track or use?
No
Testing instructions:
TBD