Spacing Support: Update padding support to allow configurable sides #30607
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
aaronrobertshaw
added
[Type] Enhancement
This was referenced Apr 8, 2021
|
Size Change: +137 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Apr 8, 2021
youknowriad
reviewed
Apr 8, 2021
| @@ -6,13 +6,20 @@ import { | |||
| __experimentalBoxControl as BoxControl, | |||
There was a problem hiding this comment.
For another time in the future, it looks like the global styles UI could share a lot of code with the hooks (not specific to this PR). cc @nosolosw
There was a problem hiding this comment.
On the topic of sharing, there could be some benefit for the block support hooks to be able to access the default theme.json style values e.g. to reset controls correctly. From memory, I don't think these values are currently available within the post editor's state like they are in the site editor.
So if a RangeControl is reset it might just be to 0 however the managed styles created from the theme.json might be 50. The result is visually the control doesn't really match what is seen in the editor.
There was a problem hiding this comment.
youknowriad
reviewed
Apr 8, 2021
This was referenced Apr 8, 2021
aaronrobertshaw
force-pushed
the
update/padding-support-with-configurable-sides
branch
from
fac2ed0
to
f6668f7
Compare
aaronrobertshaw
force-pushed
the
update/padding-support-with-configurable-sides
branch
from
f6668f7
to
786f915
Compare
Base automatically changed from
update/boxcontrol-with-configurable-sides
to
trunk
April 19, 2021 11:02
|
This could use a rebase, now that we merged the configurable sides for the component. |
aaronrobertshaw
force-pushed
the
update/padding-support-with-configurable-sides
branch
from
786f915
to
99995d3
Compare
youknowriad
reviewed
Apr 21, 2021
youknowriad
reviewed
Apr 21, 2021
I've updated the All that is left I believe is to get a confirmation on what should be changed regarding |
This includes the addition of spacing.js which is a slight refactor so the padding support is no longer an ad-hoc addition within style.js and matches the approach with colors, typography, borders etc.
aaronrobertshaw
force-pushed
the
update/padding-support-with-configurable-sides
branch
from
c4144eb
to
d3588ea
Compare
|
This has been rebased after the changes on trunk to padding.js and spacing-panel.js fixing custom CSS units. |
youknowriad
approved these changes
Apr 23, 2021
| label={ __( 'Padding' ) } | ||
| units={ units } | ||
| /> | ||
| { showPaddingControl && ( |
There was a problem hiding this comment.
Should we hide the panel entirely if there's no padding control?
There was a problem hiding this comment.
Unless I've missed something, we already do.
Within global-styles-sidebar.js there is a check using useHasSpacingPanel as to whether the SpacingPanel should be displayed. This matches the current approach for the typography, color and border panels.
The showPaddingControl check here within the component is because we'll soon be adding a control for margins as well.
There was a problem hiding this comment.
I guess I was just wondering why this component (taken separately) renders an empty panel if padding is not shown (I'm sure this will change with the addition of margins though). This is not that important.
oandregal
reviewed
Apr 27, 2021
oandregal
reviewed
Apr 27, 2021
oandregal
reviewed
Apr 27, 2021
|
The only "blocker" for me would be to clarify https://github.com/WordPress/gutenberg/pull/30607/files#r621172978 |
7 tasks
|
I see there are some conflicts but they're simply import issues. |
|
Just discovered the "conflicts" tool from GitHub so I went ahead and fixed those conflicts ― it's nice! This is ready to merge once the tests pass. |
aaronrobertshaw
deleted the
update/padding-support-with-configurable-sides
branch
|
Thanks for all the help on this 🙇 Hopefully by proxy this has resolved a lot of the issues for adding margin support in #30608 and it should be an easier review. |
7 tasks
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.
Description
This builds upon #30606 which updates the BoxControl to allow configurable sides.
In this PR, the spacing support is updated to accept this new configuration. Its inclusion into the
hooks/style.jsis refactored a little as well to bring it in line with other block supports and smooth the addition of margin support in the next PR of this chain.How has this been tested?
Manually.
Test Instructions
"padding": [ "top", "bottom" ]Test Instruction - Global Styles
core/groupblock context opting into padding support but only for top/bottom sides.stylesto addcore/groupblock context, specifying padding values.Screenshots
Types of changes
New feature. Enhancement.
Checklist:
*.native.jsfiles for terms that need renaming or removal).