Skip to content
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

Closed

Conversation

glendaviesnz
Copy link

@glendaviesnz glendaviesnz commented Jun 21, 2022

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

  • Add a Buttons block with one child Button block
  • On parent Buttons block set alignment to centre and turn off the wrapping option
  • Check that is-content-justification-center and is-nowrap classes are added in editor and frontend

Screenshots or screencast

Screen Shot 2022-06-02 at 11 17 33 AM

Trac ticket: https://core.trac.wordpress.org/ticket/56058

Copy link
Contributor

@peterwilsoncc peterwilsoncc left a 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.

src/wp-includes/block-supports/layout.php Outdated Show resolved Hide resolved
src/wp-includes/block-supports/layout.php Outdated Show resolved Hide resolved
src/wp-includes/block-supports/layout.php Outdated Show resolved Hide resolved
Comment on lines 144 to 149
* 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.
Copy link
Contributor

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

Copy link
Author

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-' );
Copy link
Contributor

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?

Copy link
Author

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.

Copy link
Contributor

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.

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?

Copy link
Author

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.

Copy link
Author

@glendaviesnz glendaviesnz Jun 23, 2022

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;
Copy link
Contributor

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.

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.

Copy link
Contributor

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.

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 🙂

@glendaviesnz
Copy link
Author

A few notes inline.

Thanks, @peterwilsoncc. I will try and get all these things sorted first thing tomorrow.

Copy link

@andrewserong andrewserong left a 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;

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.

@glendaviesnz
Copy link
Author

Also there is a mix of classname and class name, let's normalise to two words.

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
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a 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.

@glendaviesnz
Copy link
Author

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?

@samikeijonen
Copy link

samikeijonen commented Jun 23, 2022

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.

  • Yes, we need utility classes back.
  • We still need to be able to remove inline styles.

@SergeyBiryukov
Copy link
Member

Thanks for the PR! Merged in https://core.trac.wordpress.org/changeset/53569.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants