-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Enhance the shortcode block to support previewing of shortcodes #4710
Enhance the shortcode block to support previewing of shortcodes #4710
Conversation
This commit is towards rendering a front end preview for shortcode blocks. The shortcode block now implements a tabbed preview option, similar to HTML blocks. The user can edit their shortcodes, and previewing again will re-render the edited shortcode. Works for embed shortcodes too. Known issues - (1) playlist shortcode doesn't work (2) the iframe height/width in the preview tab needs to wrap content size. For example, the iframe is too big when previewing an audio player using audio shortcode (3) gallery shortcode preview stacks the images vertically instead of horizontally (4) video shortcode doesn't work for URLs supported by oembed
Previously, the post ID was fetched by processing the post's URL and parsing the post ID GET parameter. Now, a better approach is taken by reading the post ID value from the redux store.
In this revision, custom css and js files are injected as props to the iframe sandbox. Custom css and js is needed in certain cases, for example, [gallery] shortcode needs the parent theme style.css.
Shortcode content type (if it's a video or otherwise), and the shortcode's custom css and js (if any) are fetched and returned as parameters to the front end. In the case of [gallery] and [caption], for example, the theme's style.css is needed, else the shortcode preview will not render properly. [playlist] needs mediaelement JS to be able to render the playlist components.
1) Certain shortcodes, such as [gallery], [caption] and [playlist] need styles and JS inside of the sandbox iframe to be able to render content appropriately. The sandbox component didn't support injection of custom links for external stylesheets and scripts, so this revision attempts to fix that 2) [audio] shortcode fails to render since bounding client width is 0 (weird, I know). So, in this revision, I add a null check and set a minimum width if bounding client width or height is zero.
Good suggestion! We can add a message that guides users to use the embed block. The other points you've mentioned have been mostly addressed.
Exactly what I had in mind. I've tried this already, however there still seems to be an issue with [playlist]. I've described it in the "Known Issues" section in the PR description. |
[playlist] shortcode was previously not rendering in the preview due to some missing scripts. This commit fixes this issue. Try using [playlist] in the shortcode block and enjoy the preview.
Sorry for the noob question, since I'm new to jest. Requesting some pointers to fix the Travis failure - thought it would be quicker to resolve this with help from experienced jest users. I understand it is caused, in the tests, by rendering a component wrapped in UPDATE : I've fixed this. |
The jest tests failed since I was using a connected component in Shortcode, which caused the tests to act up. Followed the recommended fix and updated the snapshot to reflect the new UI for the shortcode block.
The shortcode preview result is cached using Transients, for quicker render times and avoiding processing overheads
Empty results are possible if the shortcode is given invalid attributes. In this case, we needn't unnecessarily store the empty string shortcode in transients, so I've added a check to take care of this scenario.
If shortcode output is empty, I've added a flow wherein we exit from the API immediately, and avoid some extra processing which won't apply to empty shortcode output data.
blocks/library/shortcode/index.js
Outdated
import { addQueryArgs } from '@wordpress/url'; | ||
import BlockControls from '../../block-controls'; | ||
import { getCurrentPostId } from '../../../editor/store/selectors'; | ||
import { connect } from 'react-redux'; |
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.
Might be good to separate the external, internal and WordPress dependencies like we do in other files
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.
Agree, I will separate them out. Thanks for bringing to my attention.
blocks/library/shortcode/index.js
Outdated
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import './editor.scss'; | ||
import PlainText from '../../plain-text'; | ||
|
||
export class Shortcode extends Component { |
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.
Having a hard time figuring out everything here :) do you think we can separate the rendering of the preview into a separate component ShortcodePreview
taking the content and the post id as a prop and showing the preview? It could use withAPIData
to fetch the preview.
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.
Thanks for the feedback @youknowriad . Sure, will try to refactor this piece to a separate component.
I was just looking up the withAPIData
component, and at how to send query params ( since I need to send the shortcode and postId to the REST API). The latest-posts block , which uses this, seems to have a problem. I get a 404 error. Do you see this on your build too? (I have rebased against master to have the latest version). Attaching below screenshot of what my Chrome network dev tool sees:
I'm assuming this to not be a bug with withAPIData
component, but nevertheless bringing to your attention to confirm
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.
Sometimes issues like that happen if you use "plain links" for permalinks, try using something else.
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.
@youknowriad Yes, issue was with plain links. Changed permalinks in settings and it works now.
I've refactored code, as suggested by you to use a ShortcodePreview
component. Also, I've addressed the other feedback comments you'd shared. Thanks for your time in reviewing and sharing feedback.
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.
Also, just a note - all tests have passed in my local system. Travis is having issues currently (I was told in #core-editor slack), hence the failure.
UPDATE : Travis is back, all checks passed
@@ -162,8 +174,11 @@ export default class Sandbox extends Component { | |||
<style dangerouslySetInnerHTML={ { __html: style } } /> | |||
</head> | |||
<body data-resizable-iframe-connected="data-resizable-iframe-connected" className={ this.props.type }> | |||
<div dangerouslySetInnerHTML={ { __html: this.props.html } } /> | |||
<div id="content" dangerouslySetInnerHTML={ { __html: this.props.html } } /> |
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.
why do we need a separate div for the html, js and styles can't we just concat everything when passing the html prop?
Added a couple of messages in cases when null values are sent as args to the API.
wp_footer(); | ||
$footer_scripts_styles = ob_get_clean(); | ||
|
||
// Check if shortcode is returning a video. The video type will be used by the frontend to maintain 16:9 aspect ratio. |
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.
I don't know why this is still here, but it doesn't seem to offer anything over applying the the_content
filter. Can you tell me about why you prefer to retain this method and having a "type"?
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.
@Lewiscowles1986 Are you referring to why I'm determining the type
? I'm finding out the type since sandboxing video content sets the height, as mentioned here. I see that you're always returning the type
value as html - so this condition is never met?
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.
That condition isn't for shortcode
component though. You're correct that condition of the sandbox
component isn't met, because there is no certainty the front-end will enforce a 16:9 aspect ratio. For me it's all about trying to get as close to 1:1 expectations
'style' => $style, | ||
'js' => $js, | ||
'html' => $output, | ||
'head_scripts_styles' => $head_scripts_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.
this is great but the schema at the bottom of the file is still reflecting style
and js
names
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.
I've updated the schema, thanks.
'type' => 'html', | ||
'style' => '', | ||
'js' => '', | ||
'html' => '<p>\xe2\x82\xa1</p>', |
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.
As with the plain-english version, if the text sent in matches the text output with autop applied (I tried removing the filter from the_content, damn thing is like the flu it wouldn't go away), then should that be an error-case?
I've found a problem... The rest controller should only call do_shortcode, not any other filters. The reason is simple. The shortcode block does not handle oEmbed content outside of explicit shortcode when embedding in a page. Unless the standard WP core / Gutenberg shortcode block outputs url's with an oEmbed provider in a shortcode block, simply calling |
$embed_request['url'] = $matches[5][0]; | ||
$embed_response = rest_do_request( $embed_request ); | ||
if ( $embed_response->is_error() ) { | ||
// Convert to a WP_Error object. |
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.
The error handling here won't work. Also, why are we wp_die()
ing?
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.
TBF I had hoped that the REST API handled this as I glanced at it yesterday
Should we be handling links within previewed shortcodes better? For example, I get odd behaviour with the |
@ehg could you share an example of what you mean? |
Yeah, that's probably a good point. However, it's important that the add_shortcode( 'embed', array( $wp_embed, 'shortcode' ) );
$content = do_shortcodes( … );
// Restore shortcodes. The block could short-circuit trying to send off a request (and show an error) if it doesn't contain a valid/recognized shortcode in the first place. |
Or rather, per $content = $wp_embed->run_shortcode( $content );
$content = do_shortcode( $content ); |
@westonruter in my fork I am running |
PoC shortcode preview re-factor into component without need for connect
|
PoC shortcode suggested edits handle embed and regular shortcodeshandle content not changing (shortcode invalid?)Known misses
|
Everything else here seems a matter of opinion, although I'd still like either preview to be default or a way found that enables me to write a plugin that triggers that behaviour (I don't care about the tabs) |
Suggestion Preview to be wrapped in a div |
public function __construct() { | ||
// @codingStandardsIgnoreLine - PHPCS mistakes $this->namespace for the namespace keyword | ||
$this->namespace = 'gutenberg/v1'; | ||
$this->rest_base = 'shortcodes'; |
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.
I think this would be better named shortcode-renderer
* @return true|WP_Error True if the request has read access, WP_Error object otherwise. | ||
*/ | ||
public function get_shortcode_output_permissions_check( $request ) { | ||
if ( ! current_user_can( 'edit_posts' ) ) { |
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.
If $args['postId']
is not empty, then it should also check if current_user_can( 'edit_post', $args['postId'] )
.
$output = ''; | ||
$args = $request->get_params(); | ||
$post = isset( $args['postId'] ) ? get_post( $args['postId'] ) : null; | ||
$shortcode = isset( $args['shortcode'] ) ? trim( $args['shortcode'] ) : ''; |
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.
These args don't seem to be defined in the schema, so they aren't getting sanitized/validated. The postId
should be validated to be an integer and refer to an existing post. The shortcode
should be validated to actually only contain a shortcode.
'type' => 'string', | ||
'required' => true, | ||
), | ||
'head_scripts_styles' => array( |
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.
Technically this will contain not just scripts and styles but anything that is output by wp_head
(and wp_footer
) below. I think this would be better named just html_head
and html_footer
, and maybe html_body
instead of html
.
Thanks again for your work on this, @niranjan-uma-shankar. Closing per #1054 (comment) |
Thanks @danielbachhuber and everyone else who helped review this PR. |
As this is now closed, I've added this to a repo so people that want to use my offshoot (linked above) can still use shortcodes with preview in Gutenberg https://github.com/CODESIGN2/gutenberg-shortcode-preview-block/releases |
Description
This PR adds the capability to preview shortcodes entered in the Shortcode block. This is similar to previews rendered by the embed block and custom HTML block. Work done in this PR references the issue #1054
Summary of changes
I've added a new REST API endpoint that converts the shortcode to its output filtered through its hooks
GET /gutenberg/v1/shortcodes
- get the filtered output for a given shortcodeI've changed the UI of the shortcode block to a tabbed component (identical to the custom HTML block). I've posted a comment on my thoughts behind this design consideration. The comment has screenshots of the UI, please check there.
The shortcode preview is embedded in a
Sandbox
component. This component (which is essentially creating an iframe) needed to support injection of custom JS and styles, since some shortcodes such as [gallery] and [playlist] need additional styles and scripts, such as the theme style or mediaelement JS. I've made the necessary changes to support this.How to test
[audio]
,[video]
,[caption]
,[wp_caption]
,[embed]
,[gallery]
,[playlist type="audio"]
,[playlist type="video"]
.(a) [audio] will render the attached audio vs [audio src="audio-source.mp3"]
(b) [gallery] will render all attached images, whereas [gallery ids="10,11,12"] will render those specific image ids.
Screenshot
Below is an example of a preview for [caption]
Known issue1. Preview for [wp-playlist] does not work. I'm trying to inject all the necessary JS into the Sandbox component (the js is fetched by the rest endpoint and can be seen in these lines of code). I get an errorUncaught TypeError: Cannot read property 'replace' of undefined
inunderscore.min.js
. Any pointers here would be appreciated.