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

Add "Latest Posts" Block #870

Merged
merged 18 commits into from
Jun 8, 2017
Merged

Add "Latest Posts" Block #870

merged 18 commits into from
Jun 8, 2017

Conversation

lamosty
Copy link
Member

@lamosty lamosty commented May 22, 2017

@mtias suggested to me to create a simple block to get latest posts with the WP REST API. It should also serve as an example for other developers so they can start experimenting with the API and create some cool blocks.

I've been studying Gutenberg (pretty good editor!) and come up with something really simple. It's still a work in progress since I want to fix a few things and maybe make it a bit better.

I'd like to also open a discussion about some of the things here:

  • naming: I prefixed it with rest-api- since we could have another 'latest posts' block without using rest api
  • new category called rest-api: are we planning to have more such components using rest api? If yes, I think it would be nice to give them their own category
  • getting the posts/using the rest api: I'm using the Backbone rest api js client as it seems to be the officially supported one. It offers a few niceties and it's quite easy to use but: isn't Backbone a bit dated? (I've never used it). Are we okay with this or should we create some custom rest api wrapper/client?

Asking for help: after saving a post with this latest posts block and then reloading the saved post, instead of having the block parsed and displayed as other blocks, it shows with the Gutenberg tags and HTML code. Can you spot what I'm doing wrong, please? It works fine when seeing the post on the front-end though.

Am I using the attributes of registerBlock properly? I use them to store the latest posts retrieved from DB but don't define them in registerBlock.

Thanks!

Nice to have/TODO

  • have some UI so users can filter the posts by number of them to show, author, categories, etc

@lamosty lamosty added [Feature] Blocks Overall functionality of blocks [Status] In Progress Tracking issues with work in progress labels May 22, 2017
@lamosty lamosty self-assigned this May 22, 2017
@lamosty lamosty requested a review from mtias May 22, 2017 16:01
@lamosty lamosty force-pushed the try/block-rest-api-posts branch 2 times, most recently from 969a0ed to ffcd2a1 Compare May 22, 2017 16:09
@youknowriad
Copy link
Contributor

youknowriad commented May 22, 2017

Thanks for doing this.

For now, this block will show the latest post at the moment where we save the block, if we create a new post after that. This new post is not included in the frontend. I think this post is a good opportunity to leverage the Server Side Rendering to address this.

@lamosty
Copy link
Member Author

lamosty commented May 22, 2017

I think this post is a good opportunity to leverage the Server Side Rendering to address this.

Yeah, I've seen your PR but haven't looked into it more. I also agree since it's an expected behavior to always display the latest posts, especially when using REST API (ie it's 'dynamic').

@mtias
Copy link
Member

mtias commented May 22, 2017

Thanks for giving this a try!

I also agree since it's an expected behavior to always display the latest posts, especially when using REST API (ie it's 'dynamic').

Definitely, this is a great example of a purely dynamic block in my book.

*/
import { registerBlock } from '../../api';

function getLatestPosts( postsToShow = 5 ) {
Copy link
Member

@mtias mtias May 22, 2017

Choose a reason for hiding this comment

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

Thinking... this could be a good example for breaking some logic into a separate file. Perhaps rest-api-latest-posts/data.js? Or maybe too much abstraction at this stage.

@@ -0,0 +1,71 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

Let's call this block just latest-posts.

@westonruter
Copy link
Member

There's good overlap here with the Recent Posts widget.

@jasmussen
Copy link
Contributor

Sweet deal, and awesome work!

I was so inspired I took a stab at the design of this, as suggested in your ToDo. Note: the following is very mockuppy and conceptual, and I expect some pushback here. The implementation is also probably something that can happen best in a separate PR, but nonetheless to keep the discussion grouped for now, here it is.

Neutral:

recent posts neutral

Selected:

recent posts selected

This leverages both the "block-first" UI of putting the most basic stuff right on the block, and the more advanced stuff in the Inspector on the right. For this concept, I arbitrarily decided to put the post date and number of posts in the inspector, but this was also done in order to grease the discussion on what we put there (see #672 ).

@mtias
Copy link
Member

mtias commented May 23, 2017

@jasmussen I like this, but I think we could also avoid it looking like a form if we just made the title an optional editable field like caption/quote-source with placeholder text instead of a label and an input.

@jasmussen
Copy link
Contributor

I like this, but I think we could also avoid it looking like a form if we just made the title an optional editable field like caption/quote-source with placeholder text instead of a label and an input.

This was my first instinct as well. I do think we'll want some further iteration here, as to what feels right. But I specifically like the gray background which is a callback to placeholders. Not only does this help the block feel distinct from the others, in that it's dynamic, but it also literally "grays" the content, implying you can't edit it directly.

@lamosty
Copy link
Member Author

lamosty commented May 23, 2017

Question: can attributes contain an object? For example, I'm storing the retrieved posts as JS objects in attributes and upon calling save, they get rendered into the comment as:

selection_302

However, it seems like Gutenberg doesn't like if attribute is "[object Object]" and doesn't render the block properly when loading a saved instance.

@youknowriad
Copy link
Contributor

@lamosty attributes should not store objects (At least it can't for now). I'd suggest you store the posts in a local state (the edit function can be a React Component). And I'd just return null in the save function right now.

@lamosty
Copy link
Member Author

lamosty commented May 24, 2017

In 497cee8, I added simple server-side rendering. However, I had to rename the component to core/latestposts from core/latest-posts because if there's a hyphen in the name, the server-side parser will ignore it. I think that's a bug and opened a new issue: #882.

Now, I have some more questions :):

  • when doing server-side rendering of a block, we have to write the render logic again and try to match it as closely as possible to how it works in JS. Are there some plans/ideas to add ability to write just one render logic and have it render on both client and server?
  • is it possible to use the same code that runs in the editor to render stuff on site's front-end? I.e. use the edit method to work on front-end? If not, are there any plans to support this?

@youknowriad
Copy link
Contributor

when doing server-side rendering of a block, we have to write the render logic again and try to match it as closely as possible to how it works in JS. Are there some plans/ideas to add ability to write just one render logic and have it render on both client and server?

Yes, @notnownikki proposed a SSR endpoint see (#780) and implemented it in the WIP pr #816
I think this endpoint should be extracted to its own PR

is it possible to use the same code that runs in the editor to render stuff on site's front-end? I.e. use the edit method to work on front-end? If not, are there any plans to support this?

For now, the frontend is just a raw HTML, saved from the editor (the savefunction can share some of the edit elements) or Server side rendered. I guess you're proposing to use a React component as an output, It's the first time I'm seeing this proposal, it raises so many questions, maybe worth creating an issue to discuss this possibility (but I don't see this for a v1 personally)


this.props.setAttributes( {
poststoshow
} );
Copy link
Contributor

@youknowriad youknowriad May 24, 2017

Choose a reason for hiding this comment

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

I have mixed feelings here, we're calling setAttributes in the constructor which could change the markup directly. I think it's better to use defaultAttributes property for this (see usage in the table block I think).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I didn't know we have defaultAttributes. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

I'm happy to put the SSR endpoint into its own PR, can start on it now if it's wanted :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm happy to put the SSR endpoint into its own PR, can start on it now if it's wanted :)

No rush, I think I'll leave the SSR part as it is now and we can improve it in a future PR. :)

@paaljoachim
Copy link
Contributor

Here are the various options that are used in the Genesis featured posts widget. I am posting it to give additional information on options that might be good to include.

genesis-featured-posts

@lamosty
Copy link
Member Author

lamosty commented May 24, 2017

Here are the various options that are used in the Genesis featured posts widget. I am posting it to give additional information on options that might be good to include.

Wow, that's quite a list!

I think I'll try to keep this simple though and try to implement @jasmussen's design for now. We can always improve. :)

@paaljoachim
Copy link
Contributor

paaljoachim commented May 24, 2017

Yeah a lot of options in there. That list is good to have as some of the options might be good to add at a later date.

* Returns a Promise with the latest posts or an error on failure.
*
* @param {Number} postsToShow
* @returns {Object}
Copy link
Member

Choose a reason for hiding this comment

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

Instead of {Object} this can be {wp.api.collections.Posts}

foreach( $recent_posts as $post ) {
$post_permalink = get_permalink( $post['ID'] );

$posts_content .= "<li><a href='{$post_permalink}'>{$post['post_title']}</a></li>\n";
Copy link
Member

Choose a reason for hiding this comment

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

Instead of $post['post_title'] this should use get_the_title( $post['ID'] ) so that filters apply.

index.php Outdated
@@ -473,3 +476,8 @@ function the_gutenberg_project() {
</div>
<?php
}

function gutenberg_load_blocks_server_side_redering() {
Copy link
Member

Choose a reason for hiding this comment

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

Typo s/redering/rendering/

<?php

function gutenberg_block_core_latest_posts( $attributes ) {
$postsToShow = 5;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be $posts_to_show?

@lamosty lamosty force-pushed the try/block-rest-api-posts branch from 497cee8 to ce45015 Compare May 25, 2017 09:42
@aduth aduth mentioned this pull request May 25, 2017
@nylen
Copy link
Member

nylen commented May 26, 2017

After #849 which added a bunch of tests for block parsing and serialization, this PR should probably be rebased against master, either because the build will fail upon merge, or because this PR can add some new tests using this structure (details here).

@lamosty
Copy link
Member Author

lamosty commented Jun 7, 2017

In c9c7146, I added a basically blank full post content fixtures. since latest posts block returns null in save, it doesn't save anything in database. This is what I added in core-latestposts.html file:

<!-- wp:core/latestposts poststoshow="5" --><!-- /wp:core/latestposts -->

But that doesn't test anything (except the poststoshow attribute). Should we instead use the HTML which gets rendered from the server-side rendering?

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Aside avoiding to store the request in the state, this looks solid to me 👍

@jasmussen
Copy link
Contributor

Nice work! 🎉

$posts_to_show = 5;

if ( array_key_exists( 'poststoshow', $attributes ) ) {
$posts_to_show = $attributes['poststoshow'];
Copy link
Member

Choose a reason for hiding this comment

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

Since there is no sanitization or validation of block attributes (yet), it should probably be done here, ensuring it is a number and that it is greater than 0, but less than (maybe) 100.

See also #886 (comment)

@lamosty
Copy link
Member Author

lamosty commented Jun 8, 2017

Thank you very much for all the great tips and reviews! @youknowriad @notnownikki @aduth @mtias @westonruter @jasmussen

I've learned quite a lot here and refreshed my JS skills. :)

@lamosty lamosty merged commit e94c30d into master Jun 8, 2017
@lamosty lamosty deleted the try/block-rest-api-posts branch June 8, 2017 09:33
@mtias
Copy link
Member

mtias commented Jun 8, 2017

Thank you @lamosty for working on this one.

@@ -9,6 +9,8 @@
* @package gutenberg
*/

define( 'GUTENBERG__PLUGIN_DIR', plugin_dir_path( __FILE__ ) );
Copy link
Member

Choose a reason for hiding this comment

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

These defines are duplicating code already in place elsewhere:

/**
* Retrieves the root plugin path.
*
* @return string Root path to the gutenberg plugin.
*
* @since 0.1.0
*/
function gutenberg_dir_path() {
return plugin_dir_path( dirname( __FILE__ ) );
}

The GUTENBERG__ (double underscore) convention is unique to the constants introduced here; others in the codebase do not use this convention. It seems unnecessary to me.

I don't have a preference on whether this is done as a function or constants (the function is pretty fast), but it needs to be consistent in approach and naming.

Also, gutenberg.php should contain the bare minimum necessary to initialize the plugin, because it is going to be rewritten during the build process. These defines should probably happen in a new lib/constants.php or similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I didn't notice that, my bad.

In a new PR, I'll fix those things.

Thanks!

function gutenberg_load_blocks_server_side_rendering() {
require_once GUTENBERG__BLOCKS_LIBRARY_DIR . '/latest-posts/index.php';
}
add_action( 'init', 'gutenberg_load_blocks_server_side_rendering' );
Copy link
Member

Choose a reason for hiding this comment

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

I think there are a few other issues here, not least that npm run package-plugin is currently broken because this file was not added to the plugin zip package. I'll open a PR to try to fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Feature] Widgets Screen The block-based screen that replaced widgets.php.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants