-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Changes from 44 commits
c3fd280
8cec7fc
9fc96f8
00abaca
3646f61
2681b28
d7971cb
3e5c6b4
80800ed
63549b4
9997daf
4095bca
aa57071
0e71158
4871b73
7f00de6
a6d250a
3a55c5a
7fdad34
a486b44
c9f11b5
5bf4ea3
861dd1c
bc9a96a
fc05936
d2143a5
6c31a5f
f6d525f
6aacd6e
54f2fb3
e4bbdb6
63cfafd
2ce2fef
3797ce8
65efa0b
1259bc9
3b506ed
9230d75
f837d0a
8237a66
0ae8100
d8ed5b2
49b2e85
4595fb4
777b304
80ae62a
d81a308
12d83a0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { withInstanceId, Dashicon } from '@wordpress/components'; | ||
import { Component } from '@wordpress/element'; | ||
import { __ } from '@wordpress/i18n'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import ShortcodePreview from './preview'; | ||
import BlockControls from '../../block-controls'; | ||
import PlainText from '../../plain-text'; | ||
|
||
export class Shortcode extends Component { | ||
constructor() { | ||
super(); | ||
this.state = { | ||
preview: false, | ||
}; | ||
} | ||
|
||
render() { | ||
const { preview } = this.state; | ||
const { instanceId, postId, setAttributes, attributes, isSelected } = this.props; | ||
const inputId = `blocks-shortcode-input-${ instanceId }`; | ||
const shortcodeContent = ( attributes.text || '' ).trim(); | ||
|
||
const controls = isSelected && ( | ||
<BlockControls key="controls"> | ||
<div className="components-toolbar"> | ||
<button | ||
className={ `components-tab-button ${ ! preview ? 'is-active' : '' }` } | ||
onClick={ () => this.setState( { preview: false } ) }> | ||
<span>{ __( 'Shortcode' ) }</span> | ||
</button> | ||
<button | ||
className={ `components-tab-button ${ preview ? 'is-active' : '' }` } | ||
onClick={ () => shortcodeContent.length && this.setState( { preview: true } ) } > | ||
<span>{ __( 'Preview' ) }</span> | ||
</button> | ||
</div> | ||
</BlockControls> | ||
); | ||
|
||
if ( preview ) { | ||
return [ | ||
controls, | ||
<ShortcodePreview key="preview" | ||
shortcode={ shortcodeContent } | ||
postId={ postId } | ||
/>, | ||
]; | ||
} | ||
|
||
return [ | ||
controls, | ||
<div className="wp-block-shortcode" key="placeholder"> | ||
<label htmlFor={ inputId }> | ||
<Dashicon icon="editor-code" /> | ||
{ __( 'Shortcode' ) } | ||
</label> | ||
<PlainText | ||
id={ inputId } | ||
value={ attributes.text } | ||
placeholder={ __( 'Write shortcode here…' ) } | ||
onChange={ ( text ) => setAttributes( { text } ) } | ||
/> | ||
</div>, | ||
]; | ||
} | ||
} | ||
|
||
export default withInstanceId( Shortcode ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { connect } from 'react-redux'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { withAPIData, Spinner, SandBox } from '@wordpress/components'; | ||
import { __ } from '@wordpress/i18n'; | ||
import { compose } from '@wordpress/element'; | ||
|
||
function ShortcodePreview( { response } ) { | ||
if ( response.isLoading || ! response.data ) { | ||
return ( | ||
<div key="loading" className="wp-block-embed is-loading"> | ||
<Spinner /> | ||
<p>{ __( 'Loading...' ) }</p> | ||
</div> | ||
); | ||
} | ||
|
||
const html = response.data.head_scripts_styles + ' ' + response.data.html + ' ' + response.data.footer_scripts_styles; | ||
return ( | ||
<figure className="wp-block-embed" key="embed"> | ||
<SandBox | ||
html={ html } | ||
title="Preview" | ||
type={ response.data.type } | ||
/> | ||
</figure> | ||
); | ||
} | ||
|
||
const applyConnect = connect( | ||
( state ) => { | ||
return { | ||
postId: state.currentPost.id, | ||
}; | ||
}, | ||
); | ||
|
||
const applyWithAPIData = withAPIData( ( props ) => { | ||
const { shortcode, postId } = props; | ||
return { | ||
response: `/gutenberg/v1/shortcodes?shortcode=${ encodeURIComponent( shortcode ) }&postId=${ postId }`, | ||
}; | ||
} ); | ||
|
||
export default compose( [ | ||
applyConnect, | ||
applyWithAPIData, | ||
] )( ShortcodePreview ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -142,12 +142,18 @@ export default class Sandbox extends Component { | |
body { | ||
margin: 0; | ||
} | ||
|
||
body.html { | ||
width: 100%; | ||
} | ||
|
||
body.video, | ||
body.video > div, | ||
body.video > div > iframe { | ||
width: 100%; | ||
height: 100%; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it going to harm anyone to apply height: 100% in all cases? I'm using it on my branch based upon your work where everything has There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Lewiscowles1986 Regarding changes done to Sandbox component - have you verified if it doesn't affect the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tested it with oEmbeds by typing a youtube URL into the shortcode block. Seemed to work There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tbf I changed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Lewiscowles1986 I was referring to the embed block - not embeds in the shortcode block. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have found a problem with the embed block though. It seems intended to only render the block once without an editor like shortcode block has I can share my commit once I push to github, it's mostly your code. |
||
} | ||
|
||
body > div > * { | ||
margin-top: 0 !important; /* has to have !important to override inline styles */ | ||
margin-bottom: 0 !important; | ||
|
@@ -163,8 +169,9 @@ 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 commentThe 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? |
||
<script type="text/javascript" dangerouslySetInnerHTML={ { __html: observeAndResizeJS } } /> | ||
|
||
</body> | ||
</html> | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,186 @@ | ||
<?php | ||
/** | ||
* Shortcode Blocks REST API: WP_REST_Shortcodes_Controller class | ||
* | ||
* @package gutenberg | ||
* @since 2.0.0 | ||
*/ | ||
|
||
/** | ||
* Controller which provides a REST endpoint for Gutenberg to preview shortcode blocks. | ||
* | ||
* @since 2.0.0 | ||
* | ||
* @see WP_REST_Controller | ||
*/ | ||
class WP_REST_Shortcodes_Controller extends WP_REST_Controller { | ||
/** | ||
* Constructs the controller. | ||
* | ||
* @since 2.0.0 | ||
* @access public | ||
*/ | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. I think this would be better named |
||
} | ||
|
||
/** | ||
* Registers the necessary REST API routes. | ||
* | ||
* @since 0.10.0 | ||
* @access public | ||
*/ | ||
public function register_routes() { | ||
// @codingStandardsIgnoreLine - PHPCS mistakes $this->namespace for the namespace keyword | ||
$namespace = $this->namespace; | ||
|
||
register_rest_route( $namespace, '/' . $this->rest_base, array( | ||
array( | ||
'methods' => WP_REST_Server::READABLE, | ||
'callback' => array( $this, 'get_shortcode_output' ), | ||
'permission_callback' => array( $this, 'get_shortcode_output_permissions_check' ), | ||
), | ||
'schema' => array( $this, 'get_public_item_schema' ), | ||
) ); | ||
} | ||
|
||
/** | ||
* Checks if a given request has access to read shortcode blocks. | ||
* | ||
* @since 2.0.0 | ||
* @access public | ||
* | ||
* @param WP_REST_Request $request Full details about the request. | ||
* @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 commentThe reason will be displayed to describe this comment to others. Learn more. If |
||
return new WP_Error( 'gutenberg_shortcode_block_cannot_read', __( 'Sorry, you are not allowed to read shortcode blocks as this user.', 'gutenberg' ), array( | ||
'status' => rest_authorization_required_code(), | ||
) ); | ||
} | ||
|
||
return true; | ||
} | ||
|
||
/** | ||
* Filters shortcode content through their hooks. | ||
* | ||
* @since 2.0.0 | ||
* @access public | ||
* | ||
* @param WP_REST_Request $request Full details about the request. | ||
* @return WP_REST_Response|WP_Error Response object on success, or WP_Error object on failure. | ||
*/ | ||
public function get_shortcode_output( $request ) { | ||
global $post; | ||
global $wp_embed; | ||
$head_scripts_styles = ''; | ||
$footer_scripts_styles = ''; | ||
$type = 'html'; | ||
$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 commentThe 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 |
||
|
||
// Initialize $data. | ||
$data = array( | ||
'html' => $output, | ||
'type' => $type, | ||
); | ||
|
||
if ( empty( $shortcode ) ) { | ||
$data['html'] = __( 'Enter something to preview', 'gutenberg' ); | ||
return rest_ensure_response( $data ); | ||
} | ||
|
||
if ( ! empty( $post ) ) { | ||
setup_postdata( $post ); | ||
} | ||
|
||
$output = trim( apply_filters( 'the_content', $shortcode ) ); | ||
|
||
if ( empty( $output ) ) { | ||
$data['html'] = __( 'Sorry, couldn\'t render a preview', 'gutenberg' ); | ||
return rest_ensure_response( $data ); | ||
} | ||
|
||
ob_start(); | ||
wp_head(); | ||
$head_scripts_styles = ob_get_clean(); | ||
|
||
ob_start(); | ||
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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Lewiscowles1986 Are you referring to why I'm determining the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That condition isn't for |
||
if ( has_shortcode( $shortcode, 'video' ) ) { | ||
$type = 'video'; | ||
} elseif ( has_shortcode( $shortcode, 'embed' ) ) { | ||
$embed_request = new WP_REST_Request( 'GET', '/oembed/1.0/proxy' ); | ||
$pattern = get_shortcode_regex(); | ||
if ( preg_match_all( '/' . $pattern . '/s', $shortcode, $matches ) ) { | ||
$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 commentThe reason will be displayed to describe this comment to others. Learn more. The error handling here won't work. Also, why are we There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
$error = $embed_response->as_error(); | ||
$message = $embed_response->get_error_message(); | ||
$error_data = $embed_response->get_error_data(); | ||
$status = isset( $error_data['status'] ) ? $error_data['status'] : 500; | ||
wp_die( printf( '<p>An error occurred: %s (%d)</p>', $message, $error_data ) ); | ||
} | ||
$embed_data = $embed_response->get_data(); | ||
} | ||
$type = $embed_data->type; | ||
} | ||
$data = array( | ||
'html' => $output, | ||
'head_scripts_styles' => $head_scripts_styles, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've updated the schema, thanks. |
||
'footer_scripts_styles' => $footer_scripts_styles, | ||
'type' => $type, | ||
); | ||
|
||
return rest_ensure_response( $data ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function seems a bit complex, I'm not confident in reviewing this code personally, might be good if someone else could take a look but can I ask how did you come up with the logic here and all the specific cases? don't we have a generic way to render all shortcodes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's what I've done:
Happy to provide more clarity if needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of doing something special for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In fact, I think that all of this function could be replaced with something that does the following: global $post;
$post = get_post( $args['postId'] );
setup_postdata();
ob_start();
?>
<!DOCTYPE html>
<html>
<head>
<?php wp_head(); ?>
</head>
<body>
<?php echo apply_filters( 'the_content', $args['shortcode'] ); ?>
<?php wp_footer(); ?>
</body>
</html>
<?php
$output = ob_get_clean(); This entire output could then be passed into the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for reviewing and providing your feedback @westonruter . I will implement your suggestion and test it thoroughly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @westonruter I just took a closer look at this method :
The method you've outlined doesn't determine type of content as video or otherwise, which means that 16:9 aspect ratio will never be set if user is trying to embed from TED or funnyordie etc. I will check if there's another way to determine the type. Will be happy to hear your take on this too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I altered the rest endpoint to only return 'html' type and this seems to work with embed or shortcode There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's also not using this rest endpoint (maybe why it has not affected the embed) |
||
} | ||
|
||
/** | ||
* Retrieves a shortcode block's schema, conforming to JSON Schema. | ||
* | ||
* @since 0.10.0 | ||
* @access public | ||
* | ||
* @return array Item schema data. | ||
*/ | ||
public function get_item_schema() { | ||
return array( | ||
'$schema' => 'http://json-schema.org/schema#', | ||
'title' => 'shortcode-block', | ||
'type' => 'object', | ||
'properties' => array( | ||
'html' => array( | ||
'description' => __( 'The block\'s content with shortcodes filtered through hooks.', 'gutenberg' ), | ||
'type' => 'string', | ||
'required' => true, | ||
), | ||
'type' => array( | ||
'description' => __( 'The filtered content type - video or otherwise', 'gutenberg' ), | ||
'type' => 'string', | ||
'required' => true, | ||
), | ||
'head_scripts_styles' => array( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
'description' => __( 'Links to style sheets and scripts to render the shortcode', 'gutenberg' ), | ||
'type' => 'string', | ||
'required' => true, | ||
), | ||
'footer_scripts_styles' => array( | ||
'description' => __( 'Links to style sheets and scripts to render the shortcode', 'gutenberg' ), | ||
'type' => 'string', | ||
'required' => true, | ||
), | ||
), | ||
); | ||
} | ||
} |
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 looks very similar to the Tabs in the HTML block, maybe not in this PR but it would be nice to extract those to a reusable
Tabs
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.
Agree!