-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Extract @wordpress/reusable-blocks from @wordpress/editor #25859
Conversation
Size Change: -2.75 kB (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
Any ideas how to address the linter warnings? It seems like all the problems stem from files untouched by this PR. |
If we refactor |
Here's half of what I mean: #25888 The trickier parts, which I haven't implemented, is making the Add to Reusable blocks button and inserter use I suppose that still leaves a little bit of code related to reusable blocks in |
Good idea @noisysocks ! I merged your proof of concept to this PR and moved these buttons to reusable-blocks package. From here it's just elbow grease and removing parts of the store bit by bit :-) |
@noisysocks I think it works! Would you please give it a spin? Also, I gave some thought to |
647650b
to
006c64c
Compare
This needs a bit more attention to fix the mobile tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OMG @adamziel! You did it! You legend! 😍
This looks amazing. I love all of the red. It's been a long time coming. I left a few notes.
packages/reusable-blocks/src/components/reusable-blocks-buttons/reusable-block-delete-button.js
Outdated
Show resolved
Hide resolved
@noisysocks A few considerations that are definitely out of scope for this PR:
|
Some of the tests here are quite flaky. This one fails every time on CI:
And the other ones are inconsistent, one run they're all green, another run a few are red. The funny thing is that in my local dev env all the tests pass consistently. It seems like this PR requires some more polishing. |
I think I figured this out. Now that we use |
It's an interesting thought. I guess "yes", because it makes the packages more "mix and match". That is, if you don't want reusable blocks in your But then taking this thought to its logical conclusion would mean we ought to move all blocks out into
It's odd that Will need to think more on this. I don't hate that we pass |
Could you please create an issue to track this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great.
Just a few small notes, plus I'm noticing that when I update an existing reusable block and click Save, the block encounters an error.
); | ||
const setIsEditing = useCallback( | ||
( value ) => { | ||
__experimentalSetEditingReusableBlock( clientId, value ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not use useState()
? An action/selector/reducer for this feels very heavy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does feel heavy! I went for this approach so that the createReusableBlock action could switch a newly created reusable block into an edit mode - we don't want that to happen automatically in any other case. I'm all ears for other ideas, but either way I don't think it should block this PR.
@@ -0,0 +1 @@ | |||
export { default as ReusableBlocksButtons } from './reusable-blocks-buttons'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking the name of this component should reflect that they're menu items that belong in a block menu. ReusableBlockMenuItems
is clear enough imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree! I'm hesitant of adding this change to this PR, it's already hard to reason about it as it is. Let's address it in a follow-up one.
packages/reusable-blocks/src/components/reusable-blocks-buttons/reusable-block-delete-button.js
Show resolved
Hide resolved
packages/reusable-blocks/src/components/reusable-blocks-buttons/reusable-block-delete-button.js
Show resolved
Hide resolved
I added a bandaid for now, the key to fixing it properly lies in #22127 |
Not necessarily. You can't use reusable blocks without |
I will follow up with PRs to address Rob's comments as soon as possible. Also this PR conflicted with 2293e34 to which I applied "accept mine" conflict resolution strategy as these are mostly the same changes. I will follow up with a PR to address the delta (local title state). |
This PR is getting very large and we can accomodate tweaks in smaller future PRs. I also believe @adamziel covered all your feedback @noisysocks (with one exception)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am approving this based on @noisysocks 's review and @adamziel 's covering of that, and also because the size of the change set is prone to hard to solve conflicts.
Description
This PR extracts the store part of reusable blocks logic from the editor package to a new, dedicated
@wordpress/reusable-blocks
package, and refactors it to use data layer instead of custom logic (props to @noisysocks).The rationale is that the widgets editor would benefit from supporting reusable blocks as seen in #22875. Unfortunately, a large chunk of reusable blocks logic lives in the
@wordpress/editor
package which was designed to support editing posts and would not make a good dependency for@wordpress/edit-widgets
(and wouldn't even work).Solves #25853
How has this been tested?
Go to posts editor, and interact with reusable blocks in the following ways:
When you're done, save the page and refresh the editor. Confirm all the changes were stored correctly and that your new reusable blocks are visible in the inserter.
Types of changes
Breaking change, but only if messing with experimental features could be considered breaking.
Checklist: