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

Remove data-align divs for themes that support layout #38613

Merged
merged 7 commits into from
Feb 14, 2022
Merged

Remove data-align divs for themes that support layout #38613

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e537502
Remove data-align divs for themes that support layout
youknowriad Feb 8, 2022
095a55c
Add the left/right margins properly
youknowriad Feb 8, 2022
778077c
Fix layout styles
youknowriad Feb 10, 2022
2fbef4d
fix wp-block class and z-index
youknowriad Feb 11, 2022
ddec7ac
Fix unit test
youknowriad Feb 11, 2022
f096a08
Fix caption styles
youknowriad Feb 11, 2022
e12ac14
Fix e2e test
youknowriad Feb 11, 2022
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
6 changes: 4 additions & 2 deletions lib/block-supports/layout.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support
if ( $content_size || $wide_size ) {
$style = "$selector > * {";
$style .= 'max-width: ' . esc_html( $all_max_width_value ) . ';';
$style .= '}';
$style = "$selector > :not(.alignleft):not(.alignright) {";
$style .= 'margin-left: auto !important;';
$style .= 'margin-right: auto !important;';
$style .= '}';
Expand All @@ -63,8 +65,8 @@ function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support
$style .= "$selector .alignfull { max-width: none; }";
}

$style .= "$selector .alignleft { float: left; margin-right: 2em; }";
$style .= "$selector .alignright { float: right; margin-left: 2em; }";
$style .= "$selector .alignleft { float: left; margin-right: 2em; margin-left: 0; }";
$style .= "$selector .alignright { float: right; margin-left: 2em; margin-right: 0; }";
if ( $has_block_gap_support ) {
$gap_style = $gap_value ? $gap_value : 'var( --wp--style--block-gap )';
$style .= "$selector > * { margin-top: 0; margin-bottom: 0; }";
Expand Down
27 changes: 24 additions & 3 deletions packages/block-editor/src/components/block-list/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,12 @@ import {
isUnmodifiedDefaultBlock,
} from '@wordpress/blocks';
import { withFilters } from '@wordpress/components';
import { withDispatch, withSelect, useDispatch } from '@wordpress/data';
import {
withDispatch,
withSelect,
useDispatch,
useSelect,
} from '@wordpress/data';
import { compose, pure, ifCondition } from '@wordpress/compose';
import { safeHTML } from '@wordpress/dom';

Expand Down Expand Up @@ -86,6 +91,10 @@ function BlockListBlock( {
onMerge,
toggleSelection,
} ) {
const themeSupportsLayout = useSelect( ( select ) => {
const { getSettings } = select( blockEditorStore );
return getSettings().supportsLayout;
}, [] );
const { removeBlock } = useDispatch( blockEditorStore );
const onRemove = useCallback( () => removeBlock( clientId ), [ clientId ] );

Expand Down Expand Up @@ -123,7 +132,13 @@ function BlockListBlock( {

// For aligned blocks, provide a wrapper element so the block can be
// positioned relative to the block column.
if ( isAligned ) {
// This is only kept for classic themes that don't support layout
// Historically we used to rely on extra divs and data-align to
// provide the alignments styles in the editor.
// Due to the differences between frontend and backend, we migrated
// to the layout feature, and we're now aligning the markup of frontend
// and backend.
if ( isAligned && ! themeSupportsLayout ) {
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
blockEdit = (
<div
className="wp-block"
Expand Down Expand Up @@ -164,7 +179,13 @@ function BlockListBlock( {

const value = {
clientId,
className,
className:
isAligned && themeSupportsLayout
? classnames(
className,
`align${ wrapperProps[ 'data-align' ] }`
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to add this as a class instead of sticking to the data attributes or inline layout styles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

inline layout styles target these classes. This matches the markup in the frontend.

)
: className,
wrapperProps: omit( wrapperProps, [ 'data-align' ] ),
isAligned,
};
Expand Down
13 changes: 9 additions & 4 deletions packages/block-editor/src/layouts/flow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,29 +117,34 @@ export default {
? `
${ appendSelectors( selector, '> *' ) } {
max-width: ${ contentSize ?? wideSize };
}

${ appendSelectors( selector, '> :not(.alignleft):not(.alignright)' ) } {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this that way the auto margins don't apply to left/right aligned blocks. Otherwise, the margins we had specifically for these blocks didn't apply at all. I think this has some small potential to impact FSE themes for aligned left/right blocks but this restores the original intent basically. I think it's fine but also curious about testing impact here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess also the "max-width" rule should be moved here maybe, let me know what you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @jasmussen did you see this, any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

To be sure I understand right, the desired behavior is that a floated image is omitted from max width and auto margins, so that it's up to a theme to decide whether it should be part of the main column, or not. Is that right?

If so, then that appears to work fairly well in this branch:

Screenshot 2022-02-10 at 12 13 07

Screenshot 2022-02-10 at 12 13 47

However, look at the first paragraph in that screenshot above, it no longer has a max-width for some reason, whereas it does in trunk:

Screenshot 2022-02-10 at 12 14 37

Based on later comments in Maggie's testing content, it sounds like this is could be correct:

Screenshot 2022-02-10 at 12 17 09

Screenshot 2022-02-10 at 12 17 15

However it is inconsistent with the editor 🤔 :
Screenshot 2022-02-10 at 12 17 43

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be sure I understand right, the desired behavior is that a floated image is omitted from max width and auto margins, so that it's up to a theme to decide whether it should be part of the main column, or not. Is that right?

Yes I want to exclude left/right aligned blocks from the auto margins and the max width. But it's not up to the theme to decide whether it should be part of the main column. It can't be part of the main column at all. If you want something to be part of the main column, you need to wrap it in a group.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, sounds good to me. Did you see the Paragraph missing a max-width, issue, though? That's present in this branch but not in trunk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it was actually a missing . it should be fixed in the last commit.

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 there are some unwanted side effects to adding this additional specificity.

In the flow layout, align wide and align full are no longer respected because the max-width rule from the default alignment now takes higher precedence:

Using emptytheme:

Before After
Screen Shot 2022-02-18 at 11 11 38 AM Screen Shot 2022-02-18 at 11 08 09 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch.

I wonder if this would work? :where(:not(.alignleft):not(.alignright))

Copy link
Contributor

Choose a reason for hiding this comment

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

@jasmussen's suggestion solves the alignment issue for me

margin-left: auto !important;
margin-right: auto !important;
}

${ appendSelectors( selector, '> [data-align="wide"]' ) } {
${ appendSelectors( selector, '> .alignwide' ) } {
max-width: ${ wideSize ?? contentSize };
}

${ appendSelectors( selector, '> [data-align="full"]' ) } {
${ appendSelectors( selector, '> .alignfull' ) } {
max-width: none;
}
`
: '';

output += `
${ appendSelectors( selector, '> [data-align="left"]' ) } {
${ appendSelectors( selector, '> .alignleft' ) } {
float: left;
margin-right: 2em;
margin-left: 0;
}

${ appendSelectors( selector, '> [data-align="right"]' ) } {
${ appendSelectors( selector, '> .alignright' ) } {
float: right;
margin-left: 2em;
margin-right: 0;
}

`;
Expand Down