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
Backport: Gutenberg 43660 - Separator color bugfix #3522
Backport: Gutenberg 43660 - Separator color bugfix #3522
Conversation
88fdbf0
to
e4bbc98
Compare
@c4rl0sbr4v0, the CI checks are failing. We'll need to fix them before we can commit this into the trunk. |
Thanks for the ping, @Mamaduka! It was just some blank spaces, fixed! |
return 'background-color' === $declaration['name']; | ||
} | ||
) | ||
); |
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 committed something similar a few months back, and learned that array_filter
goes over every item in the array. Refactoring these as foreach
s with a break
after the first found instance is a nice little optimization.
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.
Hmm there are multiple array_filter()
s iterating over the same dataset. This means the entire dataset is iterated each and every array_filter()
in this method.
Combining all of the array_filter()
circuits into one iterator, such as foreach()
, means the $declarations
array is walked 1 time instead of N times (N = each array_filter()
).
Granted, this means refactoring and re-testing the code to be more performant. Likely not something that can happen before 6.1.1-RC.
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.
@hellofromtonya I've refactored this to match your suggestions(at least as best as I understand them).
How does this look to you now? Any further improvements we can make?
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.
@dream-encode I agree with @hellofromtonya, I think we can avoid using array_filter()
here entirely and rather use a single foreach
. Worth adding that also array_values()
has a cost which we can avoid as well.
Given that #3555 is focused on optimizing performance by reducing excessive usage of array functions, it would feel counterproductive if we at the same time introduced new ones here.
IMO we can even have only a single foreach
loop for the entire method, iterating through $declarations
and checking for all three things (the first background-color
value, whether there are border-color
matches, and whether there are color
matches).
foreach ( $declarations as $declaration ) { | ||
if ( 'border-color' === $declaration['name'] ) { | ||
$border_color_matches = true; | ||
} elseif ( 'color' === $declaration['name'] ) { | ||
$text_color_matches = true; | ||
} | ||
} |
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.
Now that I look at the code, I see what you were saying @dream-encode:
In the foreach
, once it finds a single true
, then it can break out of the foreach
.
Why? The next conditional is checking if both are false
. As soon as one of the variables is true
, then that conditional will not execution. So no need to keep iterating in the foreach
.
My suggestion:
Add a break
to each:
foreach ( $declarations as $declaration ) {
if ( 'border-color' === $declaration['name'] ) {
$border_color_matches = true;
break;
} elseif ( 'color' === $declaration['name'] ) {
$text_color_matches = true;
break;
}
}
What do you think @dream-encode ?
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 a break in both would not work properly since we need to absolutely determine if both are true. Line 1959 check both. If we bail on the first one found, the other may yet still exist in the array. However, @felixarntz suggestion to check if both have already been found then break earlier is good.
$background_matches = array_values( | ||
array_filter( | ||
$declarations, | ||
static function( $declaration ) { | ||
return 'background-color' === $declaration['name']; | ||
} | ||
) | ||
); | ||
|
||
if ( isset( $background_matches[0]['value'] ) ) { | ||
$border_color_matches = false; | ||
$text_color_matches = false; | ||
|
||
foreach ( $declarations as $declaration ) { | ||
if ( 'border-color' === $declaration['name'] ) { | ||
$border_color_matches = true; | ||
} elseif ( 'color' === $declaration['name'] ) { | ||
$text_color_matches = true; | ||
} | ||
} | ||
|
||
if ( ! $border_color_matches && ! $text_color_matches ) { | ||
$declarations[] = array( | ||
'name' => 'color', | ||
'value' => $background_matches[0]['value'], | ||
); | ||
} | ||
} | ||
|
||
return $declarations; |
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.
@dream-encode @hellofromtonya This is what I had in mind in regards to my comment above:
$background_matches = array_values( | |
array_filter( | |
$declarations, | |
static function( $declaration ) { | |
return 'background-color' === $declaration['name']; | |
} | |
) | |
); | |
if ( isset( $background_matches[0]['value'] ) ) { | |
$border_color_matches = false; | |
$text_color_matches = false; | |
foreach ( $declarations as $declaration ) { | |
if ( 'border-color' === $declaration['name'] ) { | |
$border_color_matches = true; | |
} elseif ( 'color' === $declaration['name'] ) { | |
$text_color_matches = true; | |
} | |
} | |
if ( ! $border_color_matches && ! $text_color_matches ) { | |
$declarations[] = array( | |
'name' => 'color', | |
'value' => $background_matches[0]['value'], | |
); | |
} | |
} | |
return $declarations; | |
$background_color = ''; | |
$border_color_matches = false; | |
$text_color_matches = false; | |
foreach ( $declarations as $declaration ) { | |
if ( 'background-color' === $declaration['name'] && ! $background_color && isset( $declaration['value'] ) ) { | |
$background_color = $declaration['value']; | |
} elseif ( 'border-color' === $declaration['name'] ) { | |
$border_color_matches = true; | |
} elseif ( 'color' === $declaration['name'] ) { | |
$text_color_matches = true; | |
} | |
if ( $background_color && $border_color_matches && $text_color_matches ) { | |
break; | |
} | |
} | |
if ( $background_color && ! $border_color_matches && ! $text_color_matches ) { | |
$declarations[] = array( | |
'name' => 'color', | |
'value' => $background_color, | |
); | |
} | |
return $declarations; |
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.
@felixarntz yeah this is much better than my refactor. It checks things only when we need to.
In order to get this committed shortly for the 6.1.1 RC, I've opened an iteration PR to this #3608 since this PR branch couldn't be updated. |
Backport for WordPress/gutenberg#44943
Trac ticket: https://core.trac.wordpress.org/ticket/56903#ticket
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.