Verse Block: Add padding control #27579
Conversation
|
Size Change: +101 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
scruffian
requested review from
gziolo,
oandregal,
jorgefilipecosta and
jasmussen
scruffian
requested review from
ajitbohra,
chrisvanpatten,
ellatrix,
mkaz,
nerrad,
ntwb and
talldan
as code owners
scruffian
added
[Block] Verse
| @@ -94,7 +95,7 @@ export default function BoxControl( { | |||
| const handleOnReset = () => { | |||
| const initialValues = DEFAULT_VALUES; | |||
|
|
|||
| onChange( initialValues ); | |||
| onReset(); | |||
There was a problem hiding this comment.
With this change, when onReset isn't provided it will become noop which is change in the behavior. Should it run all logic instead for backward compatibility?
There was a problem hiding this comment.
This was kind of tricky, so I changed the reset function to return true so we could see if it's set or not. Let me know if you can think of a better way to do it.
|
I tested with the following saved post: <!-- wp:verse -->
<pre class="wp-block-verse">regular verse</pre>
<!-- /wp:verse -->
<!-- wp:verse {"style":{"typography":{"fontFamily":"var:preset|font-family|serif"}}} -->
<pre class="wp-block-verse" style="font-family:var(--wp--preset--font-family--serif)">another paragraph serif</pre>
<!-- /wp:verse -->
<!-- wp:verse {"textAlign":"center"} -->
<pre class="wp-block-verse has-text-align-center">center aligned verse</pre>
<!-- /wp:verse -->There is something happening with block validation when text alignment is set:
@nosolosw and @jorgefilipecosta, it something that is quite concerning. Is there a strategy for how those default styles should be applied to avoid content invalidation? |
It seems you are getting the validation error because the markup is missing the padding? The default styles for a block should be applied using block styles CSS. This does not seem correct I guess this padding should have been applied on style.scss. |
scruffian
force-pushed
the
update/verse-block-padding
branch
from
dc32817
to
74888b3
Compare
| @@ -60,7 +60,7 @@ function mode( arr ) { | |||
| * @return {string} A value + unit for the 'all' input. | |||
| */ | |||
| export function getAllValue( values = {} ) { | |||
| const parsedValues = Object.values( values ).map( parseUnit ); | |||
| const parsedValues = Object.values( values ).map( value => parseUnit( value ) ); | |||
There was a problem hiding this comment.
This is a nasty bug - without this line non-pixel units get converted to px
Co-authored-by: O André <[email protected]>
Co-authored-by: O André <[email protected]>
scruffian
force-pushed
the
update/verse-block-padding
branch
from
6eae081
to
cdc5dfe
Compare
|
@jorgefilipecosta @nosolosw @gziolo I think the issue with the validation comes about because of how the To replicate, checkout master, create a verse block and align it center. Then check out this branch. You should see the error Greg mentioned. This is coming about from a mismatch of the Original content: Generated content From what I can see this isn't directly related to the padding at all. However it seems that by adding the default padding we also generate this inline style on the block which wasn't there before. I've been digging around inside |
| @@ -10,14 +10,29 @@ | |||
| "default": "", | |||
| "__unstablePreserveWhiteSpace": true | |||
| }, | |||
| "style": { | |||
There was a problem hiding this comment.
Setting defaults in this way will make sure they are included as inline styles. Doing that will make theme.json not work as expected because theme.json does not produce inline styles but targets using a class. Also including inline styles even if the user does not change anything does not seems like a good idea. As all the verse blocks would have this markup instead of just having a single class with the style rule we would repeat it every time.
Allowing blocks to set default style properties may be a good idea. An equivalent to theme.json that blocks can use to target their own selector. In the future, I guess we should probably consider this. But I guess for now these defaults should just be added as CSS to the style.scss file.
There was a problem hiding this comment.
This is how the docs say we should do it: https://developer.wordpress.org/block-editor/developers/block-api/block-supports/#spacing
There was a problem hiding this comment.
It seems the documentation refers to something that at least for now should not be used. I will propose a PR to remove that from the documentation. Thank you for bringing our attention to this documentation isse.
The validation issue that happens when we pass markup from master to this branch does not seem related to align but to the fact that we are adding a default style in this branch. When I create a verse block in this branch, I get the following markup: On master I get the following markup for each alignment : If put the markup above from master on this branch the center and right block are considered invalid which is right because they don't have the style markup this branch includes. The left is considered valid, that is the strange part I think should be considered invalid as well. But regarding these issues, I think we should not add a default style attribute, as besides requiring deprecation logic, there are other cons. I expanded in detail in a comment #27579 (comment) and proposed a possible alternative for now. |
|
It feels like having a way to set the default padding in the block's |
|
The problem with setting this in CSS is that when a user resets the control they will expect the padding to return to zero, but it will show the CSS padding, which is super confusing. I created #27796 to track this. I guess this PR is blocked by that one. |
Hi @scruffian, |
|
I see two problems with doing it this way:
Why not show the default value (either from the block.json, or the theme.json) to the user when they reset the control? |
Hi @scruffian, this behaviours seems like a bug where some part of our code assumes padding of 0 (falsy) as undefined and does not apply the padding. If the user applies padding of 0 we should make sure padding of 0 is applied and the input field shows 0. The fact that the control assumes 0 is equal to no padding is something we need to correct.
This is something we want to explore as part of global styles work. And we will probably implement something to address this. But unfortunately, we don't have this yet it will depend on how nesting of semantic styles will work and is something a little bit complex. The default can be from the theme or from the user or some pattern and when we have semantic nesting on theme.json, the same verse block may have a padding of 1em, when on single post view and 0.5em when rendered on the post list so what default would we show for padding at the block level? The specific default of that block instance would depend from the contexts where it is nested. |
Yes I agree; I am just saying that we should update the control to reflect this setting, whatever it is. Another option would be to visually disable the control when it's not currently active. |
Another attempt at #27341
Description
This reimplements a custom padding control for the verse block. It also includes a couple of other fixes:
This is necessary to address the errors found in the previous PR: #27561
We also need to add custom units to the BoxControl, so we can support other units: #27615
How has this been tested?
In TT1 Blocks
Screenshots
There seems to be an issue with the visualizer component.
Types of changes
Checklist: