Make BlockManager component reusable
#67052
Merged
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
|
Size Change: +54 B (0%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
t-hamano
added
[Type] Code Quality
|
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. |
ramonjd
reviewed
Nov 18, 2024
There was a problem hiding this comment.
I don't have any comment on the master plan, but this PR makes sense and there are no regressions between trunk and this PR when toggling visibility of blocks in the preferences pane 👍🏻
LGTM so far
| */ | ||
| export default function BlockManager( { | ||
| blockTypes, | ||
| selectedBlockTypes, |
There was a problem hiding this comment.
I can't see any errors so it might be a small thing, but I'm wondering that, if this is going to be a shared component, do you think any data validation is appropriate?
If selectedBlockTypes isn't an array, a bunch of stuff would break.
There was a problem hiding this comment.
For now, I plan to release this component as a private component, so there shouldn't be any problems as long as it's used correctly within Gutenberg.
However, it would be better to at least have a conversion to an array, so I dealt with it in 242ba0e.
There was a problem hiding this comment.
I think we don't really do any validation for any component to be honest, so this would create a precedent that I'm not sure is good to make.
There was a problem hiding this comment.
I see, let's remove this validation for now.
|
Flaky tests detected in 242ba0e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11911466530
|
youknowriad
approved these changes
Nov 19, 2024
This reverts commit 242ba0e.
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.
Part of #62703 (comment)
What?
This PR makes the
BlockManagercomponent reusable in preparation for building a UI for the allowedBlocks setting.Why?
The current
BlockManagercomponent is tightly coupled to theshowBlockTypesandhideBlockTypesactions and cannot be used for other purposes.How?
Change the
BlockManagercomponent to receive the following three props.blockTypes: A list of blocks to display.selectedBlockTypes: A list of currently selected blocks.onChange: Called when the selected block is changed.Next, I reconstructed the current "Manage block visibility" settings as a
BlockVisibilitycomponent that wraps theBlockManagercomponent. TheBlockVisibilitycomponent controls the available blocks using theshowBlockTypesandhideBlockTypesvia theonChangeprop of theBlockManagercomponent.Testing Instructions
The "Manage block visibility" setting should continue to work as before. Most of the functionality should be guaranteed by this E2E test.