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.

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

Use WP Rest API infrastructure for previews #583

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

mattheu
Copy link
Contributor

@mattheu mattheu commented Mar 2, 2016

How about using the rest API infrastructure in core for previews?

  • I don't particularly think it has any real advantages? The way it handles sanitization of args is nice though.
  • doing a POST request to get preview does feel a bit odd. Any issues using GET instead?

Also deleted the legacy 'fetch' code which is no longer used - including tests. Becomes apparent that we don't have tests for the fetcher which we probably should - but this can be handled separately.

'thumbnailImage' => wp_create_nonce( 'shortcode-ui-get-thumbnail-image' ),
),
'urls' => array(
'preview' => home_url( '/wp-json/shortcode-ui/v1/preview' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use rest_url() here, to accommodate non-pretty permalinks as well.

@danielbachhuber
Copy link
Contributor

Looks pretty good!

I don't particularly think it has any real advantages?

Modernization is the key benefit, I think. Also, when batch GET is added to core, we'll be able to make use of it for batch previews.

doing a POST request to get preview does feel a bit odd. Any issues using GET instead?

Given we aren't performing a write operation, I think we should be using GET.

add_action( 'wp_enqueue_editor', 'shortcode_ui_action_wp_enqueue_editor' );
add_action( 'media_buttons', 'shortcode_ui_action_media_buttons' );
add_action( 'wp_ajax_bulk_do_shortcode', 'shortcode_ui_handle_ajax_bulk_do_shortcode' );
add_filter( 'wp_editor_settings', 'shortcode_ui_filter_wp_editor_settings', 10, 2 );
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we removing the singleton then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed this somewhere... but this is for a separate PR ;)

@mattheu
Copy link
Contributor Author

mattheu commented Mar 2, 2016

Removed some stuff I committed by accident.

Updated PR to use rest_url

@mattheu
Copy link
Contributor Author

mattheu commented Mar 2, 2016

batch GET

Yeah that would be really neat

@@ -68,9 +68,34 @@ private function setup_actions() {
add_action( 'admin_enqueue_scripts', array( $this, 'action_admin_enqueue_scripts' ) );
add_action( 'wp_enqueue_editor', array( $this, 'action_wp_enqueue_editor' ) );
add_action( 'wp_ajax_bulk_do_shortcode', array( $this, 'handle_ajax_bulk_do_shortcode' ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this registration, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yep

@danielbachhuber
Copy link
Contributor

@mattheu Can you fix the failed build? Looks good to me otherwise.

@goldenapples ?

@danielbachhuber danielbachhuber added this to the v0.7.0 milestone Mar 2, 2016

$responses = array();
public function handle_shortcode_preview( WP_REST_Request $request ) {
return $this->get_shortcode_preview( $request->get_param( 'query' ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what this function is doing, but we don't currently have a get_shortcode_preview() method here...

I think this functionality might be vestigial anyways, currently all previews are bulk requests even if they're only for one shortcode.

@goldenapples
Copy link
Contributor

This looks much cleaner than before. I like all the changes here, and doing this through the REST API makes it potentially discoverable for other uses.

@mattheu
Copy link
Contributor Author

mattheu commented Mar 3, 2016

Added a permission callback to the requests to check the user has the cap to edit the post.

@mattheu
Copy link
Contributor Author

mattheu commented Mar 3, 2016

TODO

  • add some tests.

@mattheu
Copy link
Contributor Author

mattheu commented Jun 27, 2016

I've added some tests for this. Should be good to go. @danielbachhuber would be good if you could give this a once over.

@westonruter
Copy link
Contributor

I have a concern about using the REST API for this. Namely, shortcodes may render differently depending on the URL that the shortcode appears on. In the Customizer, the Selective Refresh API makes requests to render partials by making an Ajax request to the actual URL that the partial (e.g. widget) appears on, and a template_redirect intercepts the request to just render the requested partials as opposed to loading the normal template. This ensures that when the partials render, they render in the expected context.

@mattheu
Copy link
Contributor Author

mattheu commented Jun 27, 2016

Hmm thats a good point - and sounds like a good workaround. This would affect the current implementation too.

@mattheu mattheu removed this from the v0.7.0 milestone Oct 31, 2016
@mattheu
Copy link
Contributor Author

mattheu commented Oct 31, 2016

Lets push this to a future release. Too big for 0.7.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants