Add padding control to the Global Styles sidebar #27154
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
oandregal
requested review from
ajitbohra,
chrisvanpatten,
ellatrix,
mkaz,
spacedmonkey and
TimothyBJacobs
as code owners
oandregal
added
[Feature] Design Tools
oandregal
changed the base branch from
master
to
update/global-styles-metadata
oandregal
changed the title
|
Size Change: +195 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
oandregal
force-pushed
the
add/padding-control-gs-sidebar
branch
from
b258cbe
to
4eeab8b
Compare
82 tasks
This was referenced Nov 27, 2020
oandregal
force-pushed
the
add/padding-control-gs-sidebar
branch
from
4eeab8b
to
be27842
Compare
|
@jorgefilipecosta this is ready for feedback/review. |
|
Hi @nosolosw I'm not able to see the padding control UI. Not sure if I'm missing something, or if we had a regression. |
oandregal
commented
Dec 21, 2020
| return ( | ||
| <PanelBody title={ __( 'Spacing' ) }> | ||
| <BoxControl | ||
| values={ paddingValues } |
There was a problem hiding this comment.
One way this global-level control is different from the block-level control is that the global-level doesn't use the visualizer to highlight the padding changes. The block-level control has a hold of the current block being edited, which is not true for the global-level control. A potential nice follow-up for this would be to highlight all the blocks of the given type being edited (in this case, group, or cover, etc).
oandregal
force-pushed
the
add/padding-control-gs-sidebar
branch
from
de66ab3
to
86f72fe
Compare
|
@jorgefilipecosta I've rebased this branch and tested with a brand new site: latest WP core environment, latest TT1-blocks (note that it was renamed recently), and this branch for the Gutenberg plugin. It still works as expected for me. Throwing out some random ideas in case it helps:
🤔 I can't think of anything else apart from the above at the moment. If none of the above works, I'm happy to help debug this. |
| $value = self::get_property_value( $context['styles'], $metadata['value'] ); | ||
| // Some properties can be shorthand properties, meaning that | ||
| // they contain multiple values instead of a single one. | ||
| if ( self::has_properties( $metadata ) ) { |
There was a problem hiding this comment.
We are producing styles like:
.wp-block-group {
padding-top: 5px;
padding-right: 5px;
padding-bottom: 5px;
padding-left: 5px;
}
Could we be smart and output something like "padding: 5px" in these cases? To avoiding sending unrequited bytes over the wire?
I guess we can use a "padding" property every time all for properties are set and the same behavior can be applied to margins borders etc.
There was a problem hiding this comment.
That'd be nice, can we get this in in a follow-up?
|
Thank you for helping me fix my local test issues @nosolosw! I'm now able to test the adding panels on global styles. |
There was a problem hiding this comment.
Hi @nosolosw,
We also need to update the escaping to recognize values with multiple properties otherwise we may throw warnings.
gutenberg/lib/class-wp-theme-json.php
Line 507 in 69bf4e6
oandregal
force-pushed
the
add/padding-control-gs-sidebar
branch
from
2c1907e
to
0850d16
Compare
Rebased to get the latest PRs merged and fixed this. |
jorgefilipecosta
approved these changes
Dec 29, 2020
| if ( is_array( $value ) ) { | ||
| $result = array(); | ||
| foreach ( $value as $subproperty => $subvalue ) { | ||
| $result_subproperty = safecss_filter_attr( "$name: $subvalue" ); |
There was a problem hiding this comment.
We are calling safecss_filter_attr with things like:
string(21) "background-color: 8px"
string(22) "background-color: 50px"
string(21) "background-color: 8px"
string(21) "background-color: 9px"
Possible to test by setting a user padding and adding var_dump( "$name: $subvalue" );before safecss_filter_attr filter call.
$name = 'background-color';
if ( 'gradient' === $property ) {
$name = 'background';
}
Name is always background-color or background we need to make sure the name also accounts for the padding properties.
There was a problem hiding this comment.
This is intentional, safecss_filter_attr does not check fine-grained semantics of the declaration, it only has 3 cases: 1) the value is fine (does not contain slashes, parenthesis, etc), 2) the value can contain gradients if the property is allowed to have gradients according to kses, and 3) the value can contain URL data if the property is considered safe for that. As per properties, as long they're is in the allowed list, things are fine.
Via theme.json we output more properties than the allowed by the kses function, for example, link color (a CSS Custom Property). I thought the current approach was simpler and equally robust than transforming every property to its CSS form except for link color, which we'd need to transform to another thing.
How do you feel about this?
There was a problem hiding this comment.
This PR doesn't change this behavior so went ahead and merge it. We can continue to discuss whether this is how we want it to behave.
There was a problem hiding this comment.
I thought the current approach was simpler and equally robust than transforming every property to its CSS form except for link color, which we'd need to transform to another thing.
How do you feel about this?
Hi @nosolosw, I think we need to update and use the correct CSS properties because using just two hardcoded properties is not something expected. E.g: A plugin may change to allow using filter safecss_filter_attr_allow_css a specific CSS only on a specific property, and with this approach, we don't respect what the plugin wants to do.
|
I'm doing some more testing, it seems like I'm running into some errors, and want to understand why (it may be due to old user theme.json before 0850d16). Leaving a comment so this isn't merged until that. |
oandregal
force-pushed
the
add/padding-control-gs-sidebar
branch
from
50d7b93
to
8f428c3
Compare
Rebased and tested fresh and everything is fine. |
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.
Depends on #27099
This PR adds the spacing panel to the Global Styles sidebar, so if any block declares support for it (at the moment of writing this, only group and cover) it'll be shown at the global level as well.
How to test
Test that the padding controls work in the site editor:
Test that the padding controls still work it the post editor: