Add content width to Group block by default #42582
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
|
Size Change: +116 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
This is testing pretty well manually for me @tellthemachines!
It looks like my existing Group blocks are preserved with their expected default setting (inherit set to false), and newly created Group blocks appear to be using inherit set to true 👍
Also, grouping blocks feels quite natural in the inherit mode, from a UX perspective it feels like it's working quite nicely.
For testing I used the following markup from trunk:
Test group block markup
<!-- wp:group -->
<div class="wp-block-group"><!-- wp:paragraph -->
<p>A paragraph</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->
<!-- wp:group {"backgroundColor":"vivid-red"} -->
<div class="wp-block-group has-vivid-red-background-color has-background"><!-- wp:paragraph -->
<p>A paragraph within an inherit: false default group block.</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->
<!-- wp:group {"backgroundColor":"luminous-vivid-orange","layout":{"contentSize":"300px"}} -->
<div class="wp-block-group has-luminous-vivid-orange-background-color has-background"><!-- wp:paragraph -->
<p>A paragraph within an inherit: false default group block with custom sizing.</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->
<!-- wp:group {"backgroundColor":"light-green-cyan","layout":{"inherit":true}} -->
<div class="wp-block-group has-light-green-cyan-background-color has-background"><!-- wp:paragraph -->
<p>A paragraph within an inherit: true default group block.</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->
<!-- wp:paragraph -->
<p>A regular paragraph.</p>
<!-- /wp:paragraph -->I've added a couple of questions out of curiosity — apologies if they're obvious questions! I struggled a little bit to follow how the logic's working — it appears to be working nicely for me in practice, but I wasn't too sure why we needed the deprecation or the useEffect to get it working, but I'm sure there's a good reason 😀
Happy to do further testing, too!
| useEffect( () => { | ||
| if ( inherit ) { | ||
| __unstableMarkNextChangeAsNotPersistent(); | ||
| setAttributes( { layout: { inherit } } ); |
There was a problem hiding this comment.
Why is this line required? It looks like inherit is retrieved from layout.attributes already, so I wasn't sure what the setAttributes call winds up doing here 🤔
There was a problem hiding this comment.
I'm probably doing something wrong elsewhere, but { layout: { inherit } } doesn't actually get saved to the block attributes without this bit of code. So without it, if you add a Group and then go into code view you can see the layout attribute isn't there. I feel there must be something I'm missing, not sure what 😅
There was a problem hiding this comment.
Ah, gotcha! I wonder if it's to do with the behaviour where a default value isn't serialized, but if we switch a value off and on so that setAttributes is explicitly called, then it is serialized 🤔
There was a problem hiding this comment.
That might be it! I'll dig around for any precedent with adding default attributes to blocks and see if there's a better way.
There was a problem hiding this comment.
Looking through block-library, I can't find another instance of an attribute having to be set on a block by default, without being tied to a specific action (e.g. in File and Image, new attributes have been added, but they are only set during upload). Not sure we'll be able to do without explicitly setting the attribute.
This is awkward, especially if new blocks want to have content width set by default 🤔
I'll continue looking into it.
| @@ -38,6 +39,79 @@ const migrateAttributes = ( attributes ) => { | |||
| }; | |||
|
|
|||
| const deprecated = [ | |||
| // Version with no default content width. | |||
There was a problem hiding this comment.
Why do we need the deprecation? The markup of the two different versions should be identical, so I wasn't sure if it needs a migration, since we don't want to make changes to existing blocks 🤔
There was a problem hiding this comment.
Again (and this might be connected to what I'm missing above?) if I don't add the migration, existing blocks with no layout set suddenly acquire a content width 😬
There was a problem hiding this comment.
Ah, excellent, thanks for confirming — at least knowing that much gives us a good base of behaviour for exploring our options 👍
This was referenced Jul 25, 2022
|
Closing as this was superseded by #42763. |
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?
Changing the Group block so new blocks always have a content width set by default
Why?
How?
Testing Instructions
Screenshots or screencast