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

Send post_id to the REST API in the ServerSideRender component within the Editor #9323

Merged
merged 8 commits into from Sep 12, 2018

Conversation

Projects
None yet
3 participants
@alexsanford
Contributor

alexsanford commented Aug 24, 2018

Description

Fixes #8178

This PR adds a wrapper around the ServerSideRender component within the @wordpress/editor package that adds a post_id parameter to the REST API call. This ensures the global $post object is set properly. Previously, it was only set when rendering to the frontend, but not when rendering for the API call.

This PR is an iteration of this previous PR: #8270

How has this been tested?

This was tested locally using a dynamic block that displayed information from the $post object, ensuring the post information was rendered properly on the frontend and in the editor using ServerSideRender.

An automated test was added to ensure that the parameter is added properly to the URL path.

Also I verified that the current two blocks using ServerSideRender (Archives and Latest Comments) still work as expected.

Types of changes

This change should be a non-breaking enhancement.

That said, I did change the name of the rendererPathWithAttributes function to rendererPath, and it's technically possible that some 3rd-party code is importing and using that. But I assume that's ok since it's not part of Gutenberg's documented API.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@jorgefilipecosta

This is looking good 👍 Thank you for iterating on this @alexsanford! I left some comments with possible improvements we can do but from the functional point of view, the tests went great.

@alexsanford

This comment has been minimized.

Show comment
Hide comment
@alexsanford

alexsanford Sep 7, 2018

Contributor

@jorgefilipecosta Thanks for the comments! I've addressed them above. Note that since I needed to force push anyway (because of an earlier rebase that didn't get pushed) I went ahead and rebased this onto master (no conflicts).

Contributor

alexsanford commented Sep 7, 2018

@jorgefilipecosta Thanks for the comments! I've addressed them above. Note that since I needed to force push anyway (because of an earlier rebase that didn't get pushed) I went ahead and rebased this onto master (no conflicts).

@danielbachhuber

One note to address.

@@ -20,10 +20,11 @@ import { addQueryArgs } from '@wordpress/url';
import Placeholder from '../placeholder';
import Spinner from '../spinner';
export function rendererPathWithAttributes( block, attributes = null ) {
export function rendererPath( block, attributes = null, urlQueryArgs = {} ) {

This comment has been minimized.

@danielbachhuber

danielbachhuber Sep 11, 2018

Member

Given this is an exported function, do we need to keep (or deprecate) the original rendererPathWithAttributes()?

@danielbachhuber

danielbachhuber Sep 11, 2018

Member

Given this is an exported function, do we need to keep (or deprecate) the original rendererPathWithAttributes()?

This comment has been minimized.

@danielbachhuber

danielbachhuber Sep 11, 2018

Member

Phrased another way: maybe we should keep the name as rendererPathWithAttributes(), and it simply supports a third urlQueryArgs argument.

@danielbachhuber

danielbachhuber Sep 11, 2018

Member

Phrased another way: maybe we should keep the name as rendererPathWithAttributes(), and it simply supports a third urlQueryArgs argument.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Sep 11, 2018

Member

Hi @danielbachhuber, the function rendererPathWithAttributes is exported just for testing but is not exported outside of @wordpress/components as we only export the default export outside of the module (the component)

export { default as ServerSideRender } from './server-side-render';

No plugin could have used this function so even if we change the name I don't think deprecations are necessary.

@jorgefilipecosta

jorgefilipecosta Sep 11, 2018

Member

Hi @danielbachhuber, the function rendererPathWithAttributes is exported just for testing but is not exported outside of @wordpress/components as we only export the default export outside of the module (the component)

export { default as ServerSideRender } from './server-side-render';

No plugin could have used this function so even if we change the name I don't think deprecations are necessary.

This comment has been minimized.

@danielbachhuber

danielbachhuber Sep 11, 2018

Member

Ah, makes sense. Thanks for clarifying @jorgefilipecosta

@danielbachhuber

danielbachhuber Sep 11, 2018

Member

Ah, makes sense. Thanks for clarifying @jorgefilipecosta

@jorgefilipecosta jorgefilipecosta added this to the 3.9 milestone Sep 12, 2018

@jorgefilipecosta

LGTM 👍

@jorgefilipecosta jorgefilipecosta merged commit 7dbcadb into WordPress:master Sep 12, 2018

2 checks passed

codecov/project 49.17% (-0.02%) compared to 9a6bf39
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment