Add new wrapContents prop to PluginSidebar #64273
Closed
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
|
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. |
ryanwelcher
added
[Type] Enhancement
|
Size Change: +22 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
It seems this should likely be the default and the opposite case (I need no padding at all because I'll handle stuff) to be an opt-out prop. Handling this through props is also not ideal. If there's a semantic need to have PanelBody we should encourage that explicitly. And if there is not, we should just make PluginSidebar have padding by default. Maybe the prop is more around spacing — cc @WordPress/gutenberg-components |
I agree completely but the issue is that all of the current implementations will get their UI "double wrapped" if it's opt-out. |
|
There is no semantic need to use Also, the devex isn't going to be as simple as "the container padding is enabled by default so I can just place my controls in there". There is still the problem of achieving consistent spacings between the controls. That's what Sidenote: However, the controls layout situation in the sidebar is currently a mess. Some components have bottom margins built in, which we are on the way to deprecating (#39358). On top of that, WordPress automatically applies a bottom margin on certain components (those based on |
tyxla
reviewed
Aug 6, 2024
There was a problem hiding this comment.
My $0.02: here's another group of reasons to consider this wrapContents prop unnecessary:
- The most basic
PluginSidebarexample already includesPanelBody. If there are any examples that don't use it, we could update them. - Having the panel body wrapper is achieved with basic component composability. Having the flexibility to compose components provides a better developer experience and a more expressive API than an arbitrary prop.
- This adds an extra way to add a
PanelBodywrapper, which is unnecessary ambiguity, and that would contradict the WordPress philosophy without a good reason.
|
Thanks for the feedback everyone! Based on them, I'll close this out in favor of ensuring we have better examples in the docs. |
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?
This PR is an attempt at the implement for the solution proposed in #64272 to allow developers to define a basic UI wrapper for the content of a PluginSidebar.
Why?
As mentioned in #64272 this is a much better developer experience.
How?
I have added a new
wrapContentsprop that if true, will wrap thechildrenof the<ComplementaryArea />component in a<PanelBody>component.wrapContentsis false by default for backwards compatibility with existing implementations.Testing Instructions
wrapContentsistrue, the content are wrapped (see screenshot below)wrapContentsisfalse, the content are NOT wrapped (see screenshot below)Code
wrapContents is true
wrapContents is false
st