useBlockPreview: Try syncing local style overrides with parent block editor store #55241
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewserong
added
[Type] Bug
andrewserong
requested review from
youknowriad,
apeatling,
ramonjd and
tellthemachines
andrewserong
added
the
Backport to WP 6.7 Beta/RC
|
I'm not sure how urgent this bug fix is for 6.4, but I've added the backport label just in case. Feel free to remove if folks think it's not important we get it in, or if it's too risky this close to RC 🙂 |
This was referenced Oct 11, 2023
Closed
|
Size Change: +90 B (0%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
|
I cannot speak to the technical aspects of how this is solved but I can confirm that the fix tests well for me :) The issue I reported is fixed with it applied. Also, regarding WordPress 6.4 I'm of course not one to make a call, but I personally see this as a major regression if we ship a broken Query loop block to the masses. So I would highly encourage us to backport this PR :) |
|
I might not get to test today, but +1 on backport |
|
Thanks for confirming, folks! |
|
Flaky tests detected in 5ae374e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6477966872
|
There was a problem hiding this comment.
This does fix the issue for me but it causes useBlockPreview to render three times as often even if there are no style overrides. I'm not really sure how to improve this unless we try memoizing the overrides?
I agree this should be fixed for 6.4 because the bug is really bad.
|
Thanks for testing!
It'd be a good idea to try that, though there'll likely always be some additional renders (at least one), unfortunately. I'm happy to have a go at adding some memoization tomorrow and see if that helps improve things — I might need to split out each of the props of the |
youknowriad
reviewed
Oct 11, 2023
| @@ -128,6 +161,10 @@ export function useBlockPreview( { blocks, props = {}, layout } ) { | |||
| value={ renderedBlocks } | |||
| settings={ settings } | |||
| > | |||
| <SyncStyleOverridesWithParent | |||
There was a problem hiding this comment.
Do we really need to sync here, can't we just use EditorStyles component to render these overrides here?
There was a problem hiding this comment.
I think syncing is wrong because if the "preview" is unmounted, why should its overrides linger. They are just the styles for the preview not for any parent editor.
There was a problem hiding this comment.
can't we just use EditorStyles component to render these overrides here?
Oh, good question, I'll try that out!
|
Confirmed this is working for me, although in Safari I see some rendering issues that require an extra click to clear (image half update). I didn't see any impact on images outside of the query loop. Screen.Recording.2023-10-11.at.11.15.43.AM.mp4 |
|
Thanks for the reviews and feedback, folks! I'm a little stuck on this one. Here's where I'm up to currently:
Here's a screengrab from Safari (I'm noticing much the same behaviour in both PRs) where for some reason a couple of the instances of the loop will update, but another couple will not until clicked (clicking updates the context state in the post template previews, so I imagine that might be what's forcing a re-render): 2023-10-12.15.24.44.mp4I'm not too sure what the best path forward is — perhaps if we can find a way to force the previews to re-render somehow when any of the blocks within the template are updated? Happy for any feedback or ideas, otherwise I'll continue hacking on this tomorrow! |
|
Update: I have a hunch that is not very performant but could potentially get things working properly in Safari, and that's to change the Duotone support's classname based on the current 2023-10-12.17.39.38.mp4The above is achieved by hacking the following (thank you Github Copilot — this is a super naive attempt at creating a unique classname based on attributes) into the Duotone support's const attribute = props?.attributes?.style?.color?.duotone;
const stripStringOfCharactersThatCannotBeInAClassname = ( string ) => {
return string.replace( /[^a-zA-Z0-9-]/g, '-' );
};
let suffix;
if ( Array.isArray( attribute ) ) {
suffix = attribute
.map( ( color ) => {
return stripStringOfCharactersThatCannotBeInAClassname(
color
);
} )
.join( '-' );
} else if ( typeof attribute === 'string' ) {
suffix =
stripStringOfCharactersThatCannotBeInAClassname( attribute );
}
const filterClass = `wp-duotone-${ id }-${ suffix }`;Somewhere around here: That's a pretty ugly hack for now, but I'll continue digging in in that direction tomorrow — in principle, it seems that Safari might need us to be a little heavy handed with forcing updates when Duotone styles are changed, and changing the classname based on the duotone attribute changing seems to be one way to do it. Anyhoo, I'll continue hacking tomorrow! |
There are two possible ways to solve this duplication:
|
|
Thanks for the feedback @youknowriad, that seems to have gotten the alternative PR working for me, along with some tips for @ajlende for fixing Safari! I'll close out this PR now, and we can consolidate over in #55288 and see if that PR will be viable. Thanks folks! 🙇 |
mikachan
removed
the
Backport to WP 6.7 Beta/RC
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes: #54956
This PR explores an idea to sync
useBlockPreview's local style overrides with the parent block editor store. This allows block previews that aren't rendered in an iframe (i.e. post template block's previews for non-selected blocks) to pass along their styles to the container that is outputting styles.Why?
Prior to this PR, for things that use
useBlockPreview, the styles that are generated within that preview were not propagated up to the parent environment. This is becauseExperimentalBlockEditorProvideruses its own registry, so is designed to have values passed down to it, but not to propagate values outside of itself. The proposed workaround is to pass down a setter from the parent environment to something within the block editor provider, so that we can check local state against the parent, and if there's a difference, update the parent's style overrides.How?
SyncStyleOverridesWithParentcomponent to theuseBlockPreviewhook, that takes a parent's style overrides and the parent'ssetStyleOverridesfunction as props.SyncStyleOverridesWithParent, get the local-to-the-preview's version of style overrides, and iterate over them when there's a change, and update any styles in the parent when there's a difference, based on id.Testing Instructions
Test markup
Screenshots or screencast
Before
Duotone is only applied to the selected item (as the others use a preview whose styles are not being propagated)
2023-10-11.15.17.32.mp4
After
Duotone is applied to all items, and the non selected items are updated to reflect the change:
2023-10-11.15.05.00.mp4