Add reusable blocks data layer#3017
Add reusable blocks data layer#3017youknowriad merged 4 commits intoWordPress:masterfrom noisysocks:add/reusable-blocks-data-layer
Conversation
youknowriad
left a comment
There was a problem hiding this comment.
Left some small comments, but this is in a good shape. Nice work
editor/actions.js
Outdated
There was a problem hiding this comment.
Minor: do we really need the default value?
There was a problem hiding this comment.
I suppose not! Removed in 7762cb73a84b08f60938bc004cfd1fa20bccf36a.
editor/reducer.js
Outdated
There was a problem hiding this comment.
Why are we merging the isSaving flag with the reusable block object? My first reaction here is: "Should this be in a separate reducer (or subReducer)?
There was a problem hiding this comment.
No reason other than that it's convenient to pass the block's data and metadata (isSaving) to UI components all in one go.
I'd be happy to move this to a different reducer. Perhaps the shape of the redux tree could be something like:
reusableBlocks: {
data: {
'f5ca58b0-aad0-4777-bb10-f3d0fa08a163': /* the block */,
},
isSaving: {
'f5ca58b0-aad0-4777-bb10-f3d0fa08a163': false,
},
}There was a problem hiding this comment.
Perhaps the shape of the redux tree could be something like
I like this proposal. Request flags could be generic, it makes sense to me to separate them from the data.
There was a problem hiding this comment.
👌 done in fb9d4c70a898908eddcf9b71028d16262d171791.
editor/reducer.js
Outdated
There was a problem hiding this comment.
Is there a reason we're still cloning the object?
editor/reducer.js
Outdated
There was a problem hiding this comment.
I guess the scoping ({) is not useful anymore
blocks/api/categories.js
Outdated
There was a problem hiding this comment.
These should probably be allocated in another "tab" (like recent, blocks, embeds) rather than a category. The names should probably be "Saved". cc @jasmussen
Adds the actions, selectors and reducers necessary for supporting reusable blocks. There are 5 actions: - FETCH_REUSABLE_BLOCKS: Loads reusable blocks from the API and inserts them into the store. - UPDATE_REUSABLE_BLOCK: Inserts of updates a reusable block that is in the store. - SAVE_REUSABLE_BLOCK: Persists a reusable block that's in the store to the API. - MAKE_BLOCK_STATIC: Transforms a reusable block on the page into a regular block. - MAKE_BLOCK_REUSABLE: Transforms a regular block on the page into a reusable block.
Re-shape the Redux state tree so that metadata like `isSaving` is kept seperately from each reusable block's actual data.
Codecov Report
@@ Coverage Diff @@
## master #3017 +/- ##
=========================================
+ Coverage 31.06% 31.3% +0.24%
=========================================
Files 245 246 +1
Lines 6716 6741 +25
Branches 1205 1209 +4
=========================================
+ Hits 2086 2110 +24
- Misses 3895 3896 +1
Partials 735 735
Continue to review full report at Codecov.
|
| */ | ||
| export function createReusableBlock( type, attributes ) { | ||
| return { | ||
| id: uuid(), |
There was a problem hiding this comment.
The apparent overlap yet disconnect between a reusable block's id property and a standard block's uid property is unfortunate. I can see why we might want to call this one id, since it aligns well with how it is modeled on the server (the post's ID), but it makes me wonder whether we should then align the standard block property to id as well. I'm not strongly attached to uid as a property name.
Do you have any thoughts?
There was a problem hiding this comment.
I'd be into aligning them so long as we're explicit when writing code that interacts with both types of ID, e.g:
const blockId = block.id;
const reusableBlockId = block.attributes.ref;This would be a breaking change though, since there might be third party code that references block.uid.
| export function createReusableBlock( type, attributes ) { | ||
| return { | ||
| id: uuid(), | ||
| name: __( 'Untitled block' ), |
There was a problem hiding this comment.
Would there ever be a case where we want to create a reusable block and assign its name immediately? A bit awkward to achieve now, since name is not accepted as an argument of the function.
There was a problem hiding this comment.
No such case was in our design so I figured you aren't gonna need it. We could add an optional argument in the future if it comes up.
There was a problem hiding this comment.
I think we should support this and have UI that focus on the name input before saving. cc @jasmussen
Description
Adds the actions, selectors and reducers necessary for supporting reusable blocks. There are 5 actions:
FETCH_REUSABLE_BLOCKS—Loads reusable blocks from the API and inserts them into the store.UPDATE_REUSABLE_BLOCK—Inserts and updates a reusable block that is in the store.SAVE_REUSABLE_BLOCK—Persists a reusable block that's in the store to the API.MAKE_BLOCK_STATIC—Transforms a reusable block on the page into a regular block.MAKE_BLOCK_REUSABLE—Transforms a regular block on the page into a reusable block.#1516 describes the feature and #2081 (#2081 (comment), in particular) describes the technical approach I'm taking.
This PR is a subset of #2659, which was too large. Check it out though if you'd like to play with the complete feature!
How Has This Been Tested?
Unit tests are included for the new actions, selectors and reducers.
I changed the inserter to ignore blocks that are marked with
isPrivate: true, so it's worth testing the inserter to make sure that there are no regressions.cc. @mtias @aduth @youknowriad
❤️