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

Replaces getLatestPosts usage (wp.api.collections.Posts wrapper) with withAPIData HoC #4046

Merged
merged 2 commits into from Dec 19, 2017

Conversation

Projects
None yet
2 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Dec 15, 2017

Description

This PR replaces getLatestPosts usage (wp.api.collections.Posts wrapper) with withAPIData HoC in latestPosts. It makes the block more inline with the rest of the codebase.
In order to make this task easier urlSerialize utils function was implemented. This function receives a javascript object and serializes it for usage in the URL of HTTP requests. It is handy in conjunction with withAPIData, so we don't need to be generation strings we just need to build an object.

How Has This Been Tested?

Use the latest posts block to try to change the options in the query panel and verify things work as expected.

Types of changes

Removed getLatestPosts definition and related logic in latest posts block. In the render function only the first line was changed "const latestPosts = this.props.latestPosts.data;", the rest of the function was not changed, although it appears in diff because it was moved to another file.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Dec 18, 2017

Member

In order to make this task easier urlSerialize utils function was implemented. This function receives a javascript object and serializes it for usage in the URL of HTTP requests.

Could we use Node's querystring.stringify for this?

Member

aduth commented Dec 18, 2017

In order to make this task easier urlSerialize utils function was implemented. This function receives a javascript object and serializes it for usage in the URL of HTTP requests.

Could we use Node's querystring.stringify for this?

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Dec 18, 2017

Member

Could we use Node's querystring.stringify for this?

We can probably use an equivalent of node.js querystring, like https://github.com/sindresorhus/query-string. As we were just needing a serialization (when I started it looked very simple) and I was not predicting the need for the other features like parsing, I thought adding a utils function would be a better option instead of adding a new dependency. But I can try using an external version.

Member

jorgefilipecosta commented Dec 18, 2017

Could we use Node's querystring.stringify for this?

We can probably use an equivalent of node.js querystring, like https://github.com/sindresorhus/query-string. As we were just needing a serialization (when I started it looked very simple) and I was not predicting the need for the other features like parsing, I thought adding a utils function would be a better option instead of adding a new dependency. But I can try using an external version.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Dec 18, 2017

Member

I thought adding a utils function would be a better option instead of adding a new dependency.

The Node built-in modules are stubbed by Webpack, so we shouldn't need to explicitly add a dependency for it.† I think in cases where we can leverage an existing solid Node / npm solution, we shouldn't go out of our way to reinvent the wheel unless we can benefit from a simplified, WordPress-specific offering.

† That said, the default stubbed querystring can be quite large, at least compared to some other smaller alternatives like querystringify.

Member

aduth commented Dec 18, 2017

I thought adding a utils function would be a better option instead of adding a new dependency.

The Node built-in modules are stubbed by Webpack, so we shouldn't need to explicitly add a dependency for it.† I think in cases where we can leverage an existing solid Node / npm solution, we shouldn't go out of our way to reinvent the wheel unless we can benefit from a simplified, WordPress-specific offering.

† That said, the default stubbed querystring can be quite large, at least compared to some other smaller alternatives like querystringify.

jorgefilipecosta added some commits Dec 18, 2017

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Dec 18, 2017

Member

Hi @aduth, I updated this PR, now it uses querystringify. I added a pickBy when compared to the previous version because the lib used value 'undefined' for undefined keys.

Member

jorgefilipecosta commented Dec 18, 2017

Hi @aduth, I updated this PR, now it uses querystringify. I added a pickBy when compared to the previous version because the lib used value 'undefined' for undefined keys.

@aduth

aduth approved these changes Dec 18, 2017

There's a warning when toggling "Display post date", but I'm also seeing it on master, so not likely introduced by these changes:

react-dom.d3299c8d.js:894 Warning: A component is changing an uncontrolled input of type checkbox to be controlled. Input elements should not switch from uncontrolled to controlled (or vice versa). Decide between using a controlled or uncontrolled input element for the lifetime of the component. More info: https://fb.me/react-controlled-components

@jorgefilipecosta jorgefilipecosta merged commit c4c8e3d into master Dec 19, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the update/use-withAPIData-latest-posts branch Dec 19, 2017

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