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.

Sign up for GitHub

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

Jump to bottom

Introduce prepend_to_selector() to avoid additional if checks and follow single responsibility principle #50266

Merged
merged 4 commits into from
May 23, 2023
Merged

Introduce prepend_to_selector() to avoid additional if checks and follow single responsibility principle #50266

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7e8cc35
Introduce prepend_to_selector() method to avoid additional if checks …
felixarntz May 2, 2023
45473d9
Don't call append_to_selector() when selector is likely empty.
felixarntz May 2, 2023
b9c0312
Remove parameter completely instead of deprecating.
felixarntz May 3, 2023
32332f8
Correct doc block.
felixarntz May 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 40 additions & 11 deletions lib/class-wp-theme-json-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,20 +811,46 @@ protected static function sanitize( $input, $valid_block_names, $valid_element_n
*
* @since 5.8.0
* @since 6.1.0 Added append position.
* @since 6.3.0 Deprecated append position parameter.
*
* @param string $selector Original selector.
* @param string $to_append Selector to append.
* @param string $position A position sub-selector should be appended. Default 'right'.
* @param string $selector Original selector.
* @param string $to_append Selector to append.
* @param string $deprecated Deprecated parameter.
* @return string The new selector.
*/
protected static function append_to_selector( $selector, $to_append, $position = 'right' ) {
protected static function append_to_selector( $selector, $to_append, $deprecated = 'right' ) {
spacedmonkey marked this conversation as resolved.
Show resolved Hide resolved
if ( ! str_contains( $selector, ',' ) ) {
return 'right' === $position ? $selector . $to_append : $to_append . $selector;
return $selector . $to_append;
}
$new_selectors = array();
$selectors = explode( ',', $selector );
foreach ( $selectors as $sel ) {
$new_selectors[] = 'right' === $position ? $sel . $to_append : $to_append . $sel;
$new_selectors[] = $sel . $to_append;
}
return implode( ',', $new_selectors );
}

/**
* Prepends a sub-selector to an existing one.
*
* Given the compounded $selector "h1, h2, h3"
* and the $to_prepend selector ".some-class " the result will be
* ".some-class h1, .some-class h2, .some-class h3".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted in a Slack DM to @felixarntz:

A $to_prepend of ".some-class " will produce: .some-class h1,.some-class h2,.some-class h3.

This was the result prior to this PR though. See this 3v4l.

One way to achieve the result in the docblock is trim( $sel ) and implode( ', ', $new_selectors ). See this 3v4l.

I'm not sure if these "misplaced" spaces were already known and accepted, or if this is something tests should have caught.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @costdev, good catch!

We could add trim() to the function, but to me that seems like a separate conversation. The output with trim would be preferable, but it's also not a big deal to have the extra space, and since the function is called tons of times there may be some (probably insignificant) performance cost to it.

In other words, I'm by no means opposed, but potentially this could be discussed in a separate issue / PR.

I have fixed the doc block to include the double spaces to clarify that.

Copy link
Contributor

@costdev costdev May 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a plan @felixarntz!

For a separate issue / PR, as an alternative to the trim(), if we know that the format always includes a space after the comma, we could explode( ', ', $selector ) and implode( ', ', $selector ). See this 3v4l.

*
* @since 6.3.0
*
* @param string $selector Original selector.
* @param string $to_prepend Selector to prepend.
* @return string The new selector.
*/
protected static function prepend_to_selector( $selector, $to_prepend ) {
if ( ! str_contains( $selector, ',' ) ) {
return $to_prepend . $selector;
}
$new_selectors = array();
$selectors = explode( ',', $selector );
foreach ( $selectors as $sel ) {
$new_selectors[] = $to_prepend . $sel;
}
return implode( ',', $new_selectors );
}
Expand Down Expand Up @@ -1530,10 +1556,13 @@ protected static function compute_preset_classes( $settings, $selector, $origins
$slugs = static::get_settings_slugs( $settings, $preset_metadata, $origins );
foreach ( $preset_metadata['classes'] as $class => $property ) {
foreach ( $slugs as $slug ) {
$css_var = static::replace_slug_in_string( $preset_metadata['css_vars'], $slug );
$class_name = static::replace_slug_in_string( $class, $slug );
$stylesheet .= static::to_ruleset(
static::append_to_selector( $selector, $class_name ),
$css_var = static::replace_slug_in_string( $preset_metadata['css_vars'], $slug );
$class_name = static::replace_slug_in_string( $class, $slug );

// $selector is often empty, so we can save ourselves the `append_to_selector()` call then.
$new_selector = '' === $selector ? $class_name : static::append_to_selector( $selector, $class_name );
$stylesheet .= static::to_ruleset(
$new_selector,
array(
array(
'name' => $property,
Expand Down Expand Up @@ -3476,7 +3505,7 @@ protected static function get_block_element_selectors( $root_selector ) {
$element_selector = array( $el_selector );
break;
}
$element_selector[] = static::append_to_selector( $el_selector, $selector . ' ', 'left' );
$element_selector[] = static::prepend_to_selector( $el_selector, $selector . ' ' );
}
$element_selectors[ $el_name ] = implode( ',', $element_selector );
}
Expand Down