-
Notifications
You must be signed in to change notification settings - Fork 384
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
Use responsive layout for blocks with alignwide class #4693
Use responsive layout for blocks with alignwide class #4693
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.
Minor suggestion, but otherwise from that it looks good!
|
||
$figure_node_classes = explode( ' ', $figure_node->getAttribute( 'class' ) ); | ||
|
||
// Changes layout to responsive for videos which have wide or full width (contains alignwide or alignfull class). | ||
$show_responsive = ( in_array( 'alignfull', $figure_node_classes, true ) || in_array( 'alignwide', $figure_node_classes, true ) ); | ||
|
||
if ( true === $show_responsive ) { | ||
$normalized_attributes['layout'] = 'responsive'; | ||
} |
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.
Minor formatting suggestion:
$figure_node_classes = explode( ' ', $figure_node->getAttribute( 'class' ) ); | |
// Changes layout to responsive for videos which have wide or full width (contains alignwide or alignfull class). | |
$show_responsive = ( in_array( 'alignfull', $figure_node_classes, true ) || in_array( 'alignwide', $figure_node_classes, true ) ); | |
if ( true === $show_responsive ) { | |
$normalized_attributes['layout'] = 'responsive'; | |
} | |
$figure_node_classes = explode( ' ', $figure_node->getAttribute( 'class' ) ); | |
// If the alignment was set to 'wide width' or 'full width', set the layout to responsive. | |
$show_responsive = ( | |
in_array( 'alignfull', $figure_node_classes, true ) || | |
in_array( 'alignwide', $figure_node_classes, true ) | |
); | |
if ( true === $show_responsive ) { | |
$normalized_attributes['layout'] = 'responsive'; | |
} |
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.
Thanks, I have formatted the code as per the suggestion.
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.
Just a couple other things:
|
||
if ( $figure_node && $figure_node->hasAttribute( 'class' ) ) { | ||
|
||
$figure_node_classes = explode( ' ', $figure_node->getAttribute( 'class' ) ); |
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.
It will be more robust to split by any whitespace because there could be newlines or other characters than spaces.
$figure_node_classes = explode( ' ', $figure_node->getAttribute( 'class' ) ); | |
$figure_node_classes = preg_split( '/\s+/', trim( $figure_node->getAttribute( 'class' ) ) ); |
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.
Thanks for the feedback. I have applied the suggested changes.
$show_responsive = ( | ||
in_array( 'alignfull', $figure_node_classes, true ) || | ||
in_array( 'alignwide', $figure_node_classes, true ) | ||
); | ||
|
||
if ( true === $show_responsive ) { |
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 can be simplified a bit:
$show_responsive = ( | |
in_array( 'alignfull', $figure_node_classes, true ) || | |
in_array( 'alignwide', $figure_node_classes, true ) | |
); | |
if ( true === $show_responsive ) { | |
if ( in_array( 'alignfull', $figure_node_classes, true ) || in_array( 'alignwide', $figure_node_classes, true ) ) { |
@westonruter I have applied the suggested changes. This PR can be reviewed again let me know if any feedback. |
I just realized that there is similar logic in the Image sanitizer: amp-wp/includes/sanitizers/class-amp-img-sanitizer.php Lines 323 to 333 in 31d720d
And see: amp-wp/includes/amp-helper-functions.php Lines 1275 to 1277 in a0ef87c
I think what is needed is to harmonize the logic being introduced in this PR with the pre-existing logic in the image sanitizer to apply to all other sanitizers as well, where there is the possibility for the sanitized element to be a block that has the For example, the logic probably needs to be accounted for in |
I noticed that gfycat embeds weren't getting sized responsively, but I believe they'll be accounted for in #4650. |
I also squashed the commits together to cherry-pick onto the 1.5 branch: 1ec497a. |
Summary
Fixes #4692
Checklist