Add layout classes to legacy Group inner container #5812
Conversation
tellthemachines
requested review from
felixarntz,
costdev and
mukeshpanchal27
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
There was a problem hiding this comment.
Thanks @tellthemachines for ping. Left some initial feedback for consideration.
| if ( method_exists( $processor, 'class_list' ) ) { | ||
| foreach ( $processor->class_list() as $class_name ) { | ||
| if ( str_contains( $class_name, 'layout' ) ) { | ||
| array_push( $layout_classes, $class_name ); |
There was a problem hiding this comment.
Not blocker: If adding a single element to an array, using $layout_classes[] = $class_name tends to be more concise and easier to read. However, if you're adding multiple elements at once, array_push() might be more suitable for that purpose.
There was a problem hiding this comment.
@tellthemachines Looks good, only one change request other than what @mukeshpanchal27 pointed out.
Fix inline comment formatting. Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
Fix inline comment formatting Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
Remove empty line Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
Add ticket number to test. Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
|
Thanks for the feedback folks! All points have been addressed now. |
There was a problem hiding this comment.
@tellthemachines LGTM mostly. There's one thing I'm wary about, please let me know what you think.
|
|
||
| if ( $processor->next_tag( array( 'class_name' => 'wp-block-group' ) ) ) { | ||
| foreach ( $processor->class_list() as $class_name ) { | ||
| if ( str_contains( $class_name, 'layout' ) ) { |
There was a problem hiding this comment.
This seems a bit error-prone to me. Shouldn't we maybe check for the more specific class names that can be there, or at least for a more specific pattern? What if any custom class name includes the word layout? It would be covered here when it probably shouldn't.
There was a problem hiding this comment.
Good point, the problem is finding one pattern that covers all classnames. We can have:
is-layout-[type]
wp-block-group-is-layout-[type]
wp-container-core-group-layout-[random number]
At most I think we could change the auto-generated class (the one with the random number) to match is-layout and check for that. Do you reckon that's worth trying?
There was a problem hiding this comment.
Yeah, I think that would be a reasonable improvement. Then we could probably look for is-layout- (since there's always a suffix if I'm not mistaken)?
There was a problem hiding this comment.
Done! Yes, there wouldn't ever be a class that ended in is-layout.
|
Committed in r57246. |
Trac ticket: https://core.trac.wordpress.org/ticket/60130
Syncs the changes from WordPress/gutenberg#56130 to core. Editor-side changes will be added with the npm package updates as usual; this PR will work without them but classname changes will only be visible in the front end.
To test:
is-layout-constrained wp-block-group-is-layout-constrainedare added to the inner container on the front end.This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.