-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Implement partially synced patterns behind an experimental flag #56235
Conversation
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/block-supports/pattern.php ❔ lib/experimental/blocks.php ❔ lib/experimental/connection-sources/index.php ❔ lib/experimental/editor-settings.php ❔ lib/experiments-page.php ❔ lib/load.php |
Size Change: +1.57 kB (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
Flaky tests detected in 8a06518. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6899621168
|
fe96a77
to
fb43ec2
Compare
5a020cf
to
6befb37
Compare
I have added a PR here that checks that this approach will scale to the partial syncing of blocks with innerBlocks |
6befb37
to
dc91763
Compare
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.
Thanks so much for all your explorations, @kevin940726!
So the crux of this approach seems to be:
- Avoid using
useEntityBlockEditor
and controlled inner blocks and instead just use normal inner blocks. - Listen to changes on the normal inner blocks using
registry.subscribe
and replicates those to the pattern block - Use a new
syncDerivedBlockAttributes
action to avoid additional undo stack entries for the changes to the pattern block's attributes.
useEffect( () => { | ||
const { getBlocks } = registry.select( blockEditorStore ); | ||
const { syncDerivedBlockAttributes } = unlock( | ||
registry.dispatch( blockEditorStore ) | ||
); | ||
let prevBlocks = getBlocks( patternClientId ); | ||
return registry.subscribe( () => { | ||
const blocks = getBlocks( patternClientId ); | ||
if ( blocks !== prevBlocks ) { | ||
prevBlocks = blocks; | ||
syncDerivedBlockAttributes( patternClientId, { | ||
dynamicContent: getDynamicContentFromBlocks( | ||
blocks, | ||
defaultValuesRef.current | ||
), | ||
} ); | ||
} | ||
}, blockEditorStore ); | ||
}, [ patternClientId, registry ] ); |
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.
Any concerns over using registry.subscribe
?
Feels like something where there's a lot of scope to explore different API ideas, like being able to subscribe to specific blocks, or proxying updates from one block to another.
Not something where we should hold back from shipping a first version though, but it'd be good to keep the conversation going.
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 think it's fine as it's what useBlockSync
uses under the hood too. It would be good to have something designed for this type of use case, but most of the other solutions I tried break the undo stack or require more wiring to make them work. I think we can ask for some help from @wordpress/data
and undo/redo history experts here. For now, I think this is an easy-to-understand enough implementation for our first version.
replaceInnerBlocks( | ||
patternClientId, | ||
applyInitialDynamicContent( | ||
initialBlocks, | ||
initialDynamicContent.current, | ||
defaultValuesRef.current | ||
) | ||
); |
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 guess it relies on the pattern source not being editable from the editor?
I can't see code that ever saves changes to the inner blocks back to the entity, so that's my assumption. I guess it means there's a risk that if #56534 isn't successful, this approach might need to be adjusted, and there's a bit of a dependency.
It does sort of make sense though that if there's no need to save to the entity from a pattern block instance that the code is completely removed, so yeah, there's that too 😄
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.
Yeah, it's under the assumption that we'll need a separate UI to edit the pattern source. This approach also breaks the current behavior, but I think that's expected from the product's vision. We'll need more UI/UX work to make it acceptable for regular users though 😅.
@kevin940726 I added the new experimental flag to this PR. |
…ynching of pattern instances (#56495) * Automatically assign block ids * Downgrade package to fix tooling * Move to the editor package and allow core/button * Fix test resolver
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.
Looks good, most of the comments are minor 😄
The main thing right now is this is behind an experiment flag and it doesn't seem to affect patterns in any way when the experiment is turned off.
I think it'd be pretty good to follow up quickly after this with #56574 after this PR is merged, even if it's a version of that PR that only targets the experimental version of the pattern block, as it makes the editing of partially synced patterns make a lot more sense.
|
||
// Register the block support. | ||
WP_Block_Supports::get_instance()->register( | ||
'pattern', |
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 wonder if this might be a bit confusing as we don't add pattern
as a support option to the block.json of paragraph. We could probably add a comment to the file about why it works this way.
Similar to the other comment, the name pattern
might also not be descriptive enough.
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 looks more specific to block bindings (previously connections), but it is indeed necessary for the Pattern block.
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 wonder if this could be called pattern-partial-syncing.php
or similar. While it does a slightly different thing to the js file of the same name, it is a little more descriptive.
'gutenberg-experiments', | ||
'gutenberg_experiments_section', | ||
array( | ||
'label' => __( 'Test partial syncing of patterns', 'gutenberg' ), |
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 think we could add a better description to this, but maybe I can do a follow-up PR that adds some more detail.
669bd94
to
728f592
Compare
return; | ||
} | ||
|
||
const id = nanoid( 6 ); |
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 a random string that is going to be generated whenever you set syncing for the attribute for the first time. Is that the final approach or do you plan to iterate over it?
By the way, other packages use uuid
package in similar cases. Anyway, anything that generates random strings is going to make harder implementing shuffling for pattern design while overriding the user provided content:
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.
Is that the final approach or do you plan to iterate over it?
It can be both! It's mentioned here: #53735 (comment).
By the way, other packages use
uuid
package in similar cases.
uuid
seems to be too long for storing in potentially every block in a pattern (they show up in the saved markup too). I think we could deprecate that package too in the future especially we can now just do window.crypto.randomUUID()
, if we decide that UUID is still better. nanoid
is only 100 bytes gzipped too so I think it's a better alternative here.
Anyway, anything that generates random strings is going to make harder implementing shuffling for pattern design while overriding the user provided content
Sorry, I don't follow. Could you elaborate why this impacts pattern shuffling? IIUC, we can still transform/convert/generate other deterministic ids independent of the ids here because it's all generated implicitly. We want these ids to be unique so that we can create one-to-one relationships when they are moved, transformed, or deleted. However, if we have a bigger picture of what we want to empower users, we can always switch to something deterministic (for instance, a combination of block order and block's name).
What?
Alternative to #55807.
This PR also includes #56495, which sets the block id using
nanoid
when the inspector control is checked.It only supports the
content
attribute for thecore/paragraph
for now. More blocks and attributes will be supported in future PRs.Why?
See #53705. The reason to explore this approach is try to avoid monkey-patching
setAttributes
to limit the pattern-related logic inside the scope of the pattern block itself.In #55807, we explored the approach of using nested registry to achieve that. That approach has a flaw of assuming only
setAttributes
could alter a block's attributes, but in theory there are more ways to do that (updateBlock
for instance). The same goes for the "reading" side of the flow, which is more complicated too.How?
This approach replaces the inner blocks of the pattern block to create an isolated entity that doesn't get saved to the post but has impact over the undo/redo history. We can then derive the
overrides
from the updated blocks and update the pattern block.Because
setAttributes
internally always creates a new undo level for each different block updates, we have to find a way to ignore some updates so that it doesn't break the undo/redo history. This PR creates a new private actionsyncDerivedBlockAttributes
that behaves just the same asupdateBlockAttributes
but completely ignores the undo/redo history.Testing Instructions
Follow the instructions in #55807.
Screenshots or screencast
Kapture.2023-12-12.at.11.57.31.mp4