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

Gutenberg: Update Related Posts block to pull real time related posts #27659

Merged
merged 7 commits into from
Oct 17, 2018

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Oct 5, 2018

Changes proposed in this Pull Request

  • Update the Related Posts block to pull real time related posts.
  • No longer parse dates using moment - use the API provided date, as it's already localized.
  • Revert example post dates to human readable format as we're no longer parsing them.

Preview

https://cloudup.com/cCqjmXeOfLT

The demo shows related posts for a draft post that has never been published, where you'll notice the following highlights:

  • Inserting a block in an empty post without a title results in displaying the default placeholder related posts.
  • Updating the title results in retrieving fresh related posts after successful autosave.
  • Updating the title results in retrieving fresh related posts after successful manual draft save.

Testing instructions

  • Sandbox public-api.wordpress.com (not needed anymore, merged already).
  • Apply D19169-code to your WP.com sandbox (not needed anymore, merged already).
  • If you run Jetpack locally:
    • Run the latest Jetpack.
    • Setup and run the Jetpack docker environment.
    • Expose the Jetpack docker environment to public using ngrok.
    • Apply REST API: Add a related posts endpoint  jetpack#10265 on your local Jetpack repo (not needed anymore, merged already).
    • Checkout this branch.
    • Build the blocks in Calypso running the following command:
      npm run sdk gutenberg client/gutenberg/extensions/presets/jetpack -- --output-dir=DIR_TO_JETPACK/jetpack/_inc/blocks where DIR_TO_JETPACK is the directory that leads to your local Jetpack.
  • If you run Jetpack on Jurassic Ninja:
  • Connect your Jetpack to WP.com.
  • Make sure to enable recommended modules after connecting Jetpack.
  • Make sure to create 10-ish posts with similar titles.
  • Edit one of the posts.
  • Insert the Related Posts block.
  • Verify that related posts are pulled accordingly and that they are updated on save/autosave.
  • Verify that in case there are no related posts, the block still falls back to the default content.

@tyxla tyxla added Jetpack [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR [Goal] Gutenberg Working towards full integration with Gutenberg [Type] Feature progress labels Oct 5, 2018
@tyxla tyxla self-assigned this Oct 5, 2018
@matticbot
Copy link
Contributor

@tyxla tyxla requested review from mapk, MichaelArestad and a team October 5, 2018 20:41
@ghost
Copy link

ghost commented Oct 5, 2018

That's a great PR description, thank you so much for your effort!

Generated by 🚫 dangerJS

@MichaelArestad
Copy link
Contributor

I'm having trouble testing this. Could you upload a short video of it in action?

@tyxla
Copy link
Member Author

tyxla commented Oct 9, 2018

Oh, sorry for that @MichaelArestad. It seems JN doesn't want to load with this branch - @oskosk do you have any idea what might be happening?

In the meantime, here's a demo (added to the PR description too):

https://cloudup.com/cCqjmXeOfLT

The demo shows related posts for a draft post that has never been published, where you'll notice the following highlights:

  • Inserting a block in an empty post without a title results in displaying the default placeholder related posts.
  • Updating the title results in retrieving fresh related posts after successful autosave.
  • Updating the title results in retrieving fresh related posts after successful manual draft save.

@tyxla
Copy link
Member Author

tyxla commented Oct 10, 2018

@MichaelArestad note that it's easier now to test this PR - see the updated testing instructions. Note that some amount of posts should be created and synced (which could sometimes take some minutes) in order for you to test the functionality.

@MichaelArestad
Copy link
Contributor

Awesome! Tested! Thanks for making it easier to test.

On my test, I added related posts blocks to each post I created as I created them. Once I had cranked out 3 or 4 with similar content, it was quite magical in how it updated.

However, one thing I did notice is that those first already published posts did not update the related posts section as more posts were added. I even tried updating it a few times, removing the blocks and readding them.

What I would expect is that the related posts are always changing as the site grows and more content is written. I expect this would happen without having to ever go back to the older posts to "update" the related posts content. Does that make sense?

@tyxla
Copy link
Member Author

tyxla commented Oct 11, 2018

@MichaelArestad thanks for testing!

Regarding the freshness of related posts - yeah, this is something I've noticed too, and it seems we will need to tackle it on an API level, so it's not related with the block work we're doing here. I've opened #27786 to keep track of it.

@MichaelArestad do you have any other design feedback before we mark this "design approved"?

@MichaelArestad
Copy link
Contributor

@MichaelArestad do you have any other design feedback before we mark this "design approved"?

No. It's awesome. #design-approved

@MichaelArestad MichaelArestad removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Oct 11, 2018
@tyxla tyxla force-pushed the try/gutenberg-related-posts-real-time branch from d98eaed to 74f1080 Compare October 12, 2018 07:29
Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

This is working quite nicely, thanks @tyxla!

Do you think we should re-fetch posts as the content changes? They seem to be fetched once when the block is loaded.


Not for this PR, but as we begin to consider targeting different environments, what do you think of adding fromApi + makeJsonSchemaParser to these apiFetch calls?

posts: response.posts,
fetchingPosts: false,
} );
} );
Copy link
Member

Choose a reason for hiding this comment

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

What if there's a problem fetching? I think apiFetch will reject, which we don't appear to handle.

Do we need to handle rejection? It looks like the placeholders will work and fetchingPosts will be stuck to true, but I'm wondering if these details are intentional.

Copy link
Member Author

@tyxla tyxla Oct 16, 2018

Choose a reason for hiding this comment

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

Good point, we're now handling fetching errors in 9f750ae

@tyxla
Copy link
Member Author

tyxla commented Oct 16, 2018

Thanks for the reviews, this is ready for another look.

@tyxla
Copy link
Member Author

tyxla commented Oct 16, 2018

Ah, also to answer your questions @sirreal:

Do you think we should re-fetch posts as the content changes? They seem to be fetched once when the block is loaded.

We already do that after any save / autosave - see https://github.com/Automattic/wp-calypso/pull/27659/files#diff-8957a4bfff8bf0279290b1093440645cR30

Not for this PR, but as we begin to consider targeting different environments, what do you think of adding fromApi + makeJsonSchemaParser to these apiFetch calls?

I've been thinking about it, but we're still far from that - we need a good, reliable, portable data system first. Maybe this use case could be a good start for experiments, but definitely in a separate PR, and probably not for the v1.

@sirreal
Copy link
Member

sirreal commented Oct 16, 2018

We already do that after any save / autosave

That is when this related posts block is saved, right? I was wondering if it makes sense to update more frequently, perhaps on revision save.

Once inserted, there's not much to do with this block, so I suspect save will happen infrequently. Consider a case where this block is part of a template and someone starts building out a post around it, the related posts will be outdated and not relevant as the post changes.

Not a blocker, just something to consider.

@sirreal
Copy link
Member

sirreal commented Oct 16, 2018

That is when this related posts block is saved, right? I was wondering if it makes sense to update more frequently, perhaps on revision save.

This seems to be completely wrong. It appears related posts are refetched on post (or revision) save just as I was suggesting.

@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 16, 2018
Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

e2e canaries had some failures, I've just re-run them.

Changes here are good, nice work 🙂

When tests are green, go ahead and 🚢 – please make sure we have some way to track translations for the placeholders.

@tyxla
Copy link
Member Author

tyxla commented Oct 17, 2018

Thanks @sirreal!

please make sure we have some way to track translations for the placeholders.

Sure - I've opened #27899 for that.

🚀

@tyxla tyxla merged commit a5c963f into master Oct 17, 2018
@tyxla tyxla deleted the try/gutenberg-related-posts-real-time branch October 17, 2018 08:13
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Goal] Gutenberg Working towards full integration with Gutenberg Jetpack
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants