-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add utility classnames back to blocks that have layout attributes specified #2840
Add utility classnames back to blocks that have layout attributes specified #2840
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few notes inline.
Also there is a mix of classname and class name, let's normalise to two words.
* Generates the utility classnames for the given blocks layout attributes. | ||
* This method was primarily added to reintroduce classnames that were removed | ||
* in the 5.9 release (https://github.com/WordPress/gutenberg/issues/38719), rather | ||
* than providing an extensive list of all possible layout classes. The plan is to | ||
* have the style engine generate a more extensive list of utility classnames which | ||
* will then replace this method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs rephrasing to better explain what it does, see get_post_class()
as an example.
The docs standard are to have
Short description of the function
Longer description after a double line break. This is to allow the docblock parser
to correctly format the data in the developer docs for wordpress.org
Docblocks should use first person singular. IE block rather than blocks.
@since x.x.x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated - let me know if you think the description is still not clear enough
@@ -170,8 +204,11 @@ function wp_render_layout_support_flag( $block_content, $block ) { | |||
$used_layout = $default_layout; | |||
} | |||
|
|||
$class_name = wp_unique_id( 'wp-container-' ); | |||
$gap_value = _wp_array_get( $block, array( 'attrs', 'style', 'spacing', 'blockGap' ) ); | |||
$container_class = wp_unique_id( 'wp-container-' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be in the function so it is wp_get_block_class_names
or somethign?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to keep it separate for now as this unique container id is different to the utility class names being generated by the new temporary method, and may or may not remain after that method is superseded by the style engine functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean wp_get_layout_classes()
is intended to be temporary?
I'd rather avoid adding it if it's know that it will be deprecated. It avoids the tax of having to maintain backward compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my perspective, I think we'd probably still wind up having the wp_get_layout_classes
function, but that the internals of it might be moved elsewhere to use a common abstraction. (So the implementation of the function might be temporary, but the existence of the function probably isn't). What do you think @glendaviesnz?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point @peterwilsoncc, I was overlooking the backwards compat issues of global public wp_
methods. I will take another look at this from that perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterwilsoncc - given the uncertain future of that method I have pulled the code back into the main method, and opened up a PR to do the same in Gutenberg
$class_names[] = 'is-nowrap'; | ||
} | ||
|
||
return $class_names; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A filter may be helpful here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would the use case be for adding a filter here? If it's effectively a boolean (so that folks can easily opt-out of injecting classnames), I could imagine that being useful for themes that want to switch off the feature.
Something I'd be slightly wary of, is if we were to introduce a filter that folks then use as a sort of API to inject other arbitrary classnames into the output. There's more (experimental) exploration in iterating on how the Layout support works in WordPress/gutenberg#40875, and I'm keen to ensure that we can continue to make changes to the structure and output of the classnames, without having to factor in whether or not the output is being mutated in some way 🤔
On the other hand, adding a filter is a compelling idea, it might wind up being that a good way to design it is to use a filter that receives the $block_attributes
and then outputs a list of classnames so that plugins can mutate it how they wish, but I wasn't sure if that might lock us in, since the feature is undergoing a fair few exploratory changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would the use case be for adding a filter here?
I was thinking of something along the lines of the body_class
and post_class
filters that allow folks to add or remove arbitrary classes (an empty array being the opt-out). So, exactly the thing you are weary of adding for while exploring the layout issues further 😉
Let's keep the filter on the todo list for now to avoid the potential for accidentally locking an API in before all the implications are considered.
Thanks for catching this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep the filter on the todo list for now to avoid the potential for accidentally locking an API in before all the implications are considered.
Excellent, thanks Peter! I've added a link to this thread in the Gutenberg tracking issue for the Layout block support (WordPress/gutenberg#39336) under the "TODO" section, so that we don't forget 🙂
Thanks, @peterwilsoncc. I will try and get all these things sorted first thing tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hope you don't mind me chiming in here @peterwilsoncc, I thought I'd take a look since I'm currently working on an exploratory refactor of the Layout support. I've just added a couple of questions about the comment about adding a filter, but keen to hear your thoughts or concerns about how it works! 🙂
$class_names[] = 'is-nowrap'; | ||
} | ||
|
||
return $class_names; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would the use case be for adding a filter here? If it's effectively a boolean (so that folks can easily opt-out of injecting classnames), I could imagine that being useful for themes that want to switch off the feature.
Something I'd be slightly wary of, is if we were to introduce a filter that folks then use as a sort of API to inject other arbitrary classnames into the output. There's more (experimental) exploration in iterating on how the Layout support works in WordPress/gutenberg#40875, and I'm keen to ensure that we can continue to make changes to the structure and output of the classnames, without having to factor in whether or not the output is being mutated in some way 🤔
On the other hand, adding a filter is a compelling idea, it might wind up being that a good way to design it is to use a filter that receives the $block_attributes
and then outputs a list of classnames so that plugins can mutate it how they wish, but I wasn't sure if that might lock us in, since the feature is undergoing a fair few exploratory changes.
It seems like there is a mix of these throughout the WP codebase already, and changing them in this file now seems to extend the scope of change beyond what is being ported across from GB - are you happy to leave it as is for now? Might be a good thing to look at standardising at some point though. |
…new, potentially temporary method to global wp_ namespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@glendaviesnz Thanks mate, I appreciate you taking my comments in the spirit intended.
No worries. Do I need to do anything else with this PR or will the 6.0.1 team sort the next steps? |
Hello, nice to see this happening! This is how agencies removes inline styles because they usually break CSS work flow: remove_filter( 'render_block', 'wp_render_layout_support_flag', 10, 2 ); Am I reading the code correctly that current solution would also remove these utility classes? If yes, there needs to be some other solution :) Different filter / function for example.
|
Thanks for the PR! Merged in https://core.trac.wordpress.org/changeset/53569. |
What?
Add several layout utility classnames to blocks that have layout attributes specified
Why?
In 5.9 these utility classnames were removed which removed the ability for theme/plugin authors to assign their own custom CSS related to specific layout selections. This was mostly related to the Button block
How?
Adds these classes dynamically based on attributes, rather than saving them to the serialized content
Testing Instructions
is-content-justification-center
andis-nowrap
classes are added in editor and frontendScreenshots or screencast
Trac ticket: https://core.trac.wordpress.org/ticket/56058