Fix Synced Patterns non-overridden content being editable in write mode #66949
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
- Remove the prev. React effect for managing pattern block editing modes
- Implement higher order reducer in the block editor store ... it:
- Tracks the clientIds of pattern blocks.
- Uses the pattern block clientIds to manage block editing modes
for pattern blocks and their inner blocks.
- Updates both on any actions that change block lists.
talldan
added
[Type] Bug
|
Size Change: +297 B (+0.02%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 4c7653a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11812251734
|
talldan
changed the title
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
youknowriad
reviewed
Nov 13, 2024
| @@ -3071,6 +3071,10 @@ export const getBlockEditingMode = createRegistrySelector( | |||
| return 'disabled'; | |||
| } | |||
|
|
|||
| if ( state?.patternBlockEditingModes?.has( clientId ) ) { | |||
| return state?.patternBlockEditingModes.get( clientId ); | |||
| } | |||
There was a problem hiding this comment.
What I find odd is that the denormalization only applies in the case of the "pattern block editing modes", why not just make a global state.blockEditingModes ?
There was a problem hiding this comment.
I see you mentioned in the description that you have follow-up for "zoom-out" editing modes too. I think we should ideally just have the selector return directly the state value in the end, but I'm ok with going in steps.
There was a problem hiding this comment.
One challenge is going to be that the selector depends on editorMode which is not within the reducer itself. So it's possible that in the end we'll have two values in the state for each block, a block editing mode for "write" and another one for "design" and the selector would pick the right value depending on the active mode.
There was a problem hiding this comment.
What I find odd is that the denormalization only applies in the case of the "pattern block editing modes", why not just make a global state.blockEditingModes ?
Yeah, it's a tricky one. In the case of switching from design to write mode, we don't need to recalculate every pattern block, so storing it separately is useful in that case, we can retain the info across editor mode switches.
Similarly, we need to retain back compat for the block editing modes that might be set via setBlockEditingMode, so I think those also need to be stored in separate state so that we don't accidentally clear them when switching editor modes.
There was a problem hiding this comment.
oh yes, I mistakenly used the same name, we do need this to be separate from the original setBlockEditingMode state.
There was a problem hiding this comment.
One challenge is going to be that the selector depends on editorMode which is not within the reducer itself.
My other PR does handle this by dispatching an action for switching editor mode to ensure the reducer re-runs:
https://github.com/WordPress/gutenberg/pull/66919/files#diff-d1fbd3af72ee0220c5c3ab2952a24d818174aed711b9a50b57c9544218a63988R1681
but it still reads the mode from the preferences store:
https://github.com/WordPress/gutenberg/pull/66919/files#diff-3c4b969f7547d02bc04a2c554e0f7d21da07c3f779714acf02800ee0341b50d9R279-R281
I'm not sure if this is the best approach. The reducer also needs access to the blocks store to check for role: content attributes, so it's not the only place where it selects from other stores and I saw there was some precedence for this in the block editor reducer.
youknowriad
reviewed
Nov 13, 2024
|
|
||
| return nextState; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Do you think this is the kind of code that deserves some unit testing?
There was a problem hiding this comment.
I can do, though I think the other PR (#66919) shows that unit testing only the reducer or only the selectors isn't a great approach. That PR is a refactor, the store as a whole should be working the same, but I'll have to rewrite all the tests, which is a lot of work and could introduce some margin of error.
There was a problem hiding this comment.
I should also add some e2e tests.
This was referenced Nov 15, 2024
|
This is superceded by #67026, which I'd be happy to receive reviews on. I plan to add some more tests to it, but I'm happy enough with the shape of the code. |
talldan
deleted the
add/higher-order-reducer-for-pattern-editing-modes
branch
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 #66424
Alternative to #65408
In Write Mode, parts of synced patterns that are supposed to be non-editable are editable.
Why?
This happens because Write Mode overrides the
blockEditingModeof a synced pattern's inner blocks.Pattern blocks try calling
setBlockEditingModefor child blocks directly, but when write mode is active a different value is returned directly from thegetBlockEditingModeselector, one that doesn't take into account how synced patterns should work.How?
This PR fixes the issue by moving the pattern block editing modes to the block editor store, so that it can work harmoniously with the way Write Mode and Zoom Out mode determine the block editing mode.
Initially #65408 attempted this by adding code directly to the getBlockEditingMode` selector, but this caused a performance issue so it was reverted. That selector is called a lot, so it's sensitive to any new code that might cause some overhead.
This PR follows the suggestion of @youknowriad - #65408 (comment).
A higher order reducer instead calculates the block editing modes whenever there are changes to the block list. This doesn't seem to cause any noticeable performance issues. The approach taken in the higher order reducer is to first track and added/removed pattern blocks (
core/blockblock types) and then build up aSetof editing modes for those blocks and their children.I also have another PR that aims to move the zoomed out/write mode code from the selector to a higher order reducer, which I can follow up on if this approach looks good.
Testing Instructions
Screenshots or screencast
Before
Kapture.2024-11-13.at.17.24.45.mp4
After
Kapture.2024-11-13.at.17.28.44.mp4