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

ServerSideRender: Allow using component without defined attributes #7229

Merged
merged 1 commit into from Jun 15, 2018

Conversation

Projects
None yet
3 participants
@dimofte
Contributor

dimofte commented Jun 8, 2018

Description

Fixes #7214
Optional attributes in ServerSideRender

How has this been tested?

Manually in Chrome on MacOS.
The component doesn't have automated tests and I haven't implemented any.

Types of changes

Arguably a bug fix or a non-breaking api change

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation. - not applicable
@chrisvanpatten

This comment has been minimized.

Contributor

chrisvanpatten commented Jun 8, 2018

Looks like the Travis failure is unrelated; some files missing that aren't related to this PR.

@@ -46,7 +46,7 @@ export class ServerSideRender extends Component {
if ( null !== this.state.response ) {
this.setState( { response: null } );
}
const { block, attributes } = props;
const { block, attributes = {} } = props;

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 14, 2018

Member

Should attributes be present in the request if it's empty?

@chrisvanpatten

This comment has been minimized.

Contributor

chrisvanpatten commented Jun 15, 2018

@danielbachhuber That's a great point. We took this approach for a quick solution with a small surface area, but it could definitely be improved.

Maybe if #7232 can be merged in its current state, we could close this PR and address it as part of the longer-term fix mentioned in #7232? If we could use a JS implementation of http_build_query like I mentioned in that issue, we could just have the whole query string generated in JS rather than just tacking on the attributes.

@danielbachhuber danielbachhuber added this to the 3.1 milestone Jun 15, 2018

@danielbachhuber danielbachhuber changed the title from ssr: accept undefined attributes to ServerSideRender: Allow using component without defined attributes Jun 15, 2018

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 15, 2018

I'm fine to land this as-is, given the developer-facing API won't change with the future enhancement.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 15, 2018

Ignoring the codecov failure because it seems to be useless / false-positive.

@danielbachhuber danielbachhuber merged commit a9c3392 into WordPress:master Jun 15, 2018

1 of 2 checks passed

codecov/project 46.08% (-0.61%) compared to a9fab4d
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