Improve Homepage Posts block editor performance #499
Conversation
|
I've tested a few cases in the editor, more with an eye to see if things were working the same way as before. Adding and removing blocks works really well, whether the blocks are added/removed at the top of the page, bottom or middle. I did run into an issue when reordering the blocks -- it looks like the posts aren't updated at all when the order is changed, though they correct themselves when doing another action that updates them (like add/delete):
I also ran into something weird when changing the number of posts per block -- for the most part, this updated pretty well, but from time to time I managed to "trick" it, and could set the number of posts per block to something that would show under Display Settings, but wouldn't be reflected in the editor. The following clip happened after changing the counts of a few of the blocks; the third block is correctly showing 2 posts from the previous change: https://cloudup.com/cAY3FIeblcF Let me know if I can provide any info about either of these! |
|
Thanks for catching those, Laurel!
|
|
Thanks Adam! Block reordering looks good now in my testing 👍
I could recreate a couple times by doing the following:
At some point, the count gets weird -- the first time I tried to recreate today, the second block I changed was set to 6 posts, but only displayed 5. The second time it took a bit more editing; after changing 4-5 blocks, I ended up with one that says it has a count of 3, but is actually displaying 7. My localhost can get pretty slow -- that could also be contributing, I'm not sure. Just let me know if it'd be more helpful to do a quick screenshare or something -- issues that are hard to recreate kind of drive me nuts, you never know if they're happening in their own weird bubble (like my slow localhost), or if there's just a really specific step that is hard to hit! |
|
I figured out a way to reproduce that:
In pt. 2 the The reason is twofold:
I've solved 2. by implementing a slightly different debouncing strategy: a1e6345 and it should work as expected now, even without the disabling of the input. |
There was a problem hiding this comment.
I've solved 2. by implementing a slightly different debouncing strategy: a1e6345 and it should work as expected now, even without the disabling of the input.
Can confirm! I hammered it a lot more than I did the first time -- changing the posts-per-block count on multiple posts before each had finished loading -- and each time they landed on the right number.
Thanks Adam! Marking as approved from a behaviour perspective, but it would probably be good to get the 👍 from Jeff, too, just to make sure everything looks OK code-wise.
|
I think we should consider removing the disabled states. From a UI perspective it's nice to show the activity, but on pages with a large number of blocks this leads to significant periods of time with locked UI. Here's a video showing this. It was taken on a Jurassic Ninja instance with approximately 300 Posts, on a Page with 27 Homepage Posts blocks. Several Newspack sites have homepages of similar or greater density. https://cloudup.com/cTmN9CMSh9V @adekbadek what do you think? |
|
Done in 1600925 . Note that just the sidebar UI controls' disabling is removed, the blocks will still be dimmed until the reflow completes. |
There was a problem hiding this comment.
Working really well! I still need another hour or two with the store but two things that can be discussed and/or addressed:
- There are a few changes here which may break the Full Site Editing import of the block. In FSE the block is registered with a different name (
a8c/blog-posts- https://github.com/Automattic/wp-calypso/blob/master/apps/full-site-editing/full-site-editing-plugin/newspack-blocks/blog-posts-block-editor.js#L19) and the store is initiated from a different place (https://github.com/Automattic/wp-calypso/blob/master/apps/full-site-editing/full-site-editing-plugin/newspack-blocks/blog-posts-block-editor.js#L37). Because of these and a few other differences, the changes in this branch are likely to break that implementation, or at least require some changes inwp-calypso. To ease integration, is it possible to keep theregisterQueryStoreand the block registration the same as before? If not, we'll just have to make sure to address inwp-calypsobefore the next deploy of FSE. - Since we've removed the disabled form element states would it hurt to also remove the disabled state of the block content too? As with the form elements, I agree that what you've done is "correct" UI but since the blocks may take a noticeable interval to update and since the changes applied mid-update are harmless, removing the state change might remove friction from the editing process. What do you think?
| registerBlockType( `newspack-blocks/${ name }`, settings ); | ||
| registerQueryStore( `newspack-blocks/${ name }` ); | ||
| registerBlockType( BLOCK_NAME, settings ); | ||
| registerHomepagePostsBlockStore(); |
There was a problem hiding this comment.
Renaming the store registration function may break the implementation in Full Site Editing, here: https://github.com/Automattic/wp-calypso/blob/master/apps/full-site-editing/full-site-editing-plugin/newspack-blocks/blog-posts-block-editor.js#L14
jeffersonrabb
added
[Status] Under Review
Needs Changes or Feedback
and removed
[Status] Approved
labels
I think that's ok – FSE renames the block, so it doesn't matter what's the original name. And this PR does not change the
Good catch, I've updated the code in d1ada72 to be backwards-compatible.
Besides the
I agree that not disabling the inputs makes for a smoother editing experience, but "dimming" of the block content does not prevent the user from editing. I think it's good feedback that the reflow is still in progress – perhaps there could be less dimming, so that it's more subtle? |
|
As @jeffersonrabb pointed out, some changes have indeed broken backwards compatibility – namely internal handling the block name. Fixed in 4ee4b33 |
src/components/query-controls.js
Outdated
| @@ -159,6 +159,7 @@ class QueryControls extends Component { | |||
| tagExclusions, | |||
| onTagExclusionsChange, | |||
| enableSpecific, | |||
| disabled, | |||
src/components/query-controls.js
Outdated
| @@ -169,6 +170,7 @@ class QueryControls extends Component { | |||
| checked={ specificMode } | |||
| onChange={ onSpecificModeChange } | |||
| label={ __( 'Choose Specific Posts', 'newspack-blocks' ) } | |||
| disabled={ disabled } | |||
src/components/query-controls.js
Outdated
| @@ -183,6 +185,7 @@ class QueryControls extends Component { | |||
| 'Begin typing post title, click autocomplete result to select.', | |||
| 'newspack-blocks' | |||
| ) } | |||
| disabled={ disabled } | |||
src/components/query-controls.js
Outdated
| @@ -194,6 +197,7 @@ class QueryControls extends Component { | |||
| fetchSuggestions={ this.fetchAuthorSuggestions } | |||
| fetchSavedInfo={ this.fetchSavedAuthors } | |||
| label={ __( 'Authors', 'newspack-blocks' ) } | |||
| disabled={ disabled } | |||
src/components/query-controls.js
Outdated
| @@ -204,6 +208,7 @@ class QueryControls extends Component { | |||
| fetchSuggestions={ this.fetchCategorySuggestions } | |||
| fetchSavedInfo={ this.fetchSavedCategories } | |||
| label={ __( 'Categories', 'newspack-blocks' ) } | |||
| disabled={ disabled } | |||
src/components/query-controls.js
Outdated
| @@ -214,6 +219,7 @@ class QueryControls extends Component { | |||
| fetchSuggestions={ this.fetchTagSuggestions } | |||
| fetchSavedInfo={ this.fetchSavedTags } | |||
| label={ __( 'Tags', 'newspack-blocks' ) } | |||
| disabled={ disabled } | |||
src/components/query-controls.js
Outdated
| @@ -236,6 +242,7 @@ class QueryControls extends Component { | |||
| fetchSuggestions={ this.fetchTagSuggestions } | |||
| fetchSavedInfo={ this.fetchSavedTags } | |||
| label={ __( 'Excluded Tags', 'newspack-blocks' ) } | |||
| disabled={ disabled } | |||
| @@ -161,7 +161,7 @@ class AutocompleteTokenField extends Component { | |||
| * Render. | |||
| */ | |||
| render() { | |||
| const { help, label = '' } = this.props; | |||
| const { help, label = '', disabled } = this.props; | |||
| @@ -172,6 +172,7 @@ class AutocompleteTokenField extends Component { | |||
| onChange={ tokens => this.handleOnChange( tokens ) } | |||
| onInputChange={ input => this.debouncedUpdateSuggestions( input ) } | |||
| label={ label } | |||
| disabled={ disabled } | |||
|
🎉 This PR is included in version 1.8.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
All Submissions:
Changes proposed in this Pull Request:
Improve the performance of Homepage Blocks post in the editor.
Closes #493
How to test the changes in this Pull Request:
nof Homepage Posts blocks, each with a category assignednnumber of requests to/v2/postsWP API endpoint**/v2/postsrequests on editor page load matches the number of blocks on the page/v2/postsrequest****. Observe that the sabotaged block displays an error, but the rest are displayed as expected. Such an error onmastershould cause an infinite loading state on the block.* I went with 10, which triggered 55 requests
** To see the no. of requests in Chrome's network tab:
*** Not all
@wordpress/componentscontrol components support thedisabledprop - changing that is in progress at WordPress/gutenberg#22701**** In Chrome network tab:
Other information: