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

Refactor block alignments and margins #20689

Closed
wants to merge 1 commit into from
Closed

Refactor block alignments and margins #20689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions packages/base-styles/_z-index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ $z-layers: (
".components-drop-zone": 40,
".components-drop-zone__content": 50,

// The block mover for floats should overlap the controls of adjacent blocks.
".block-editor-block-list__block {core/image aligned left or right}": 21,
// The flotaed blocks should be higher than regular blocks
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// The flotaed blocks should be higher than regular blocks
// The floated blocks should be higher than regular blocks

".wp-block {aligned left or right}": 21,

// Small screen inner blocks overlay must be displayed above drop zone,
// settings menu, and movers.
Expand Down
24 changes: 22 additions & 2 deletions packages/block-editor/src/components/block-list/block-wrapper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { focus, isTextField, placeCaretAtHorizontalEdge } from '@wordpress/dom';
import { BACKSPACE, DELETE, ENTER } from '@wordpress/keycodes';
import { __, sprintf } from '@wordpress/i18n';
import { useSelect, useDispatch } from '@wordpress/data';
import { hasBlockSupport } from '@wordpress/blocks';

/**
* Internal dependencies
Expand Down Expand Up @@ -49,7 +50,9 @@ const BlockComponent = forwardRef(
mode,
blockTitle,
wrapperProps,
attributes,
} = useContext( BlockContext );
const supportAlignments = hasBlockSupport( name, 'align', false );
Copy link
Member

Choose a reason for hiding this comment

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

Should this accept an array of possible alignments?

const { initialPosition } = useSelect(
( select ) => {
if ( ! isSelected ) {
Expand Down Expand Up @@ -190,7 +193,7 @@ const BlockComponent = forwardRef(
const blockElementId = `block-${ clientId }${ htmlSuffix }`;
const Animated = animated[ tagName ];

return (
const blockContentElement = (
<Animated
// Overrideable props.
aria-label={ blockLabel }
Expand All @@ -199,7 +202,9 @@ const BlockComponent = forwardRef(
{ ...props }
id={ blockElementId }
ref={ wrapper }
className={ classnames( className, props.className ) }
className={ classnames( className, props.className, {
'wp-block': ! supportAlignments,
Copy link
Member

Choose a reason for hiding this comment

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

Why does this class have to be removed when there's alignment? Does it hurt to have .wp-block > .wp-block?

} ) }
data-block={ clientId }
data-type={ name }
data-title={ blockTitle }
Expand All @@ -216,6 +221,21 @@ const BlockComponent = forwardRef(
{ children }
</Animated>
);

if ( supportAlignments ) {
return (
<div
className={ classnames( 'wp-block wp-align-wrapper', {
[ `wp-align-${ attributes.align }` ]: !! attributes.align,
Copy link
Member

Choose a reason for hiding this comment

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

Restating my concerns in #20650 (comment), I think now is the prime time to revise the naming scheme for the alignment/position/width options. wp-align-wide shouldn't be a thing because "wide" is not really an alignment, and wp-align-left is misleading if it actually refers to a float.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you propose? for me we should stay consistent in terms of classNames, in terms of labels in the UI it can be different

Copy link
Member

Choose a reason for hiding this comment

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

If we were to put them all under a single naming scheme, I'd go with something like this:

wp-position-xxx with the xxx replaced with one of the following for each of the possible values:

  • align-left (as in actual left alignment)
  • align-right
  • align-center
  • float-left (or maybe pull-left)
  • float-right (or maybe pull-right)
  • stretch-wide-width (or maybe just wide)
  • stretch-full-width (or maybe just full-width)

I'm not sure whether wp-position- is the right naming scheme, though. CSS already has a thing called position that isn't really the same concept. Perhaps it may also be worth distinguishing from potential vertical options by using wp-horizontal-position- or wp-x-position- or something like that.

But I'm not convinced that different options should use the same naming scheme if they're technically different things. If we were to use more diverse names depending on what exactly the position/alignment/whatever actually is, I would go with something like the following:

  • wp-align-left
  • wp-align-right
  • wp-align-center (or possibly wp-align-center-horizontally or wp-align-center-x to distinguish from potential vertical centering or something like that in the future)
  • wp-float-left (or maybe wp-pull-left)
  • wp-float-right (or maybe wp-pull-right)
  • wp-width-wide
  • wp-width-full

I prefer this naming scheme since it's more concise, and doesn't try to fit a redundant, possibly incorrect word like "position" into the class names.

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 don't think we can change the name of the alignments today left/right/wide/full/center are here to stay. I don't think it's worth the hassle to rename these for BC. We should try to figure out a prefix that work well for these. Otherwise, we'll just add complexity to map the alignment name with the classname without high benefits.

Also cc @aduth as my go to naming person :P

Copy link
Member

Choose a reason for hiding this comment

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

@youknowriad wp-align-xxx is already different from the existing alignxxx class names, though, so we're already updating the class names, aren't we?

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, but I'm talking about the attribute values, not the classNames.We also need to keep. BC for existing themes with data-align[value] stylesheets. Again renaming this is possible but not without needless complexity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we omit the wp- prefix for alignments?

Copy link
Member

Choose a reason for hiding this comment

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

@jasmussen I'm fine with removing them.

Copy link
Member

Choose a reason for hiding this comment

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

@youknowriad Personally, I think that changing the attribute values, even if it means some somewhat complex back-compat, is well worth it to get class names that actually make sense. Changing how block alignments work is likely to break styles anyway, so we might as well take advantage of the opportunity so people don't have to update their styles twice.

Also, do we even need data-align anymore? See #20689 (comment).

} ) }
data-align={ attributes.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 are we adding the data attributes here if we're going with classes? I think this will only make things more confusing as it's the old way of styling alignment in the editor.

>
{ blockContentElement }
</div>
);
}

return blockContentElement;
}
);

Expand Down
3 changes: 2 additions & 1 deletion 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 @@ -99,7 +99,7 @@ function BlockListBlock( {
// Generate the wrapper class names handling the different states of the
// block.
const wrapperClassName = classnames(
'wp-block block-editor-block-list__block',
'block-editor-block-list__block',
{
'has-selected-ui': hasSelectedUI,
'has-warning': ! isValid || !! hasError || isUnregisteredBlock,
Expand Down Expand Up @@ -164,6 +164,7 @@ function BlockListBlock( {
mode,
blockTitle: blockType.title,
wrapperProps,
attributes,
};
const memoizedValue = useMemo( () => value, Object.values( value ) );

Expand Down
50 changes: 0 additions & 50 deletions packages/block-editor/src/components/block-list/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,56 +247,6 @@
cursor: default;
}

// Alignments.
&[data-align="left"],
&[data-align="right"] {
// Without z-index, won't be clickable as "above" adjacent content.
z-index: z-index(".block-editor-block-list__block {core/image aligned left or right}");
width: 100%;

// When images are floated, the block itself should collapse to zero height.
height: 0;

&::before {
content: none;
}
}

// Left.
&[data-align="left"] > .is-block-content {
// This is in the editor only; the image should be floated on the frontend.
/*!rtl:begin:ignore*/
float: left;
margin-right: 2em;
/*!rtl:end:ignore*/
}

// Right.
&[data-align="right"] > .is-block-content {
// Right: This is in the editor only; the image should be floated on the frontend.
/*!rtl:begin:ignore*/
float: right;
margin-left: 2em;
/*!rtl:end:ignore*/
}

// Wide and full-wide.
&[data-align="full"],
&[data-align="wide"] {
clear: both;
}

// Full-wide.
&[data-align="full"] {
margin-left: -$block-padding;
margin-right: -$block-padding;

@include break-small() {
margin-left: -$block-padding - $block-padding - $block-side-ui-width - $border-width - $border-width;
margin-right: -$block-padding - $block-padding - $block-side-ui-width - $border-width - $border-width;
}
}

// Clear floats.
&[data-clear="true"] {
float: none;
Expand Down
15 changes: 11 additions & 4 deletions packages/block-editor/src/hooks/align.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,10 @@ export function addAttribute( settings ) {
if ( has( settings.attributes, [ 'align', 'type' ] ) ) {
return settings;
}
if ( hasBlockSupport( settings, 'align' ) ) {
if (
hasBlockSupport( settings, 'align' ) &&
! hasBlockSupport( settings, 'lightBlockWrapper' )
) {
// Use Lodash's assign to gracefully handle if attributes are undefined
settings.attributes = assign( settings.attributes, {
align: {
Expand Down Expand Up @@ -124,7 +127,8 @@ export const withToolbarControls = createHigherOrderComponent(
const validAlignments = isEmbedButton
? []
: getValidAlignments(
getBlockSupport( blockName, 'align' ),
getBlockSupport( blockName, 'align' ) &&
! hasBlockSupport( blockName, 'lightBlockWrapper' ),
hasBlockSupport( blockName, 'alignWide', true )
);

Expand Down Expand Up @@ -182,7 +186,8 @@ export const withDataAlign = createHigherOrderComponent(
}

const validAlignments = getValidAlignments(
getBlockSupport( name, 'align' ),
getBlockSupport( name, 'align' ) &&
! hasBlockSupport( name, 'lightBlockWrapper' ),
hasBlockSupport( name, 'alignWide', true ),
hasWideEnabled
);
Expand All @@ -207,7 +212,9 @@ export const withDataAlign = createHigherOrderComponent(
*/
export function addAssignedAlign( props, blockType, attributes ) {
const { align } = attributes;
const blockAlign = getBlockSupport( blockType, 'align' );
const blockAlign =
getBlockSupport( blockType, 'align' ) &&
! hasBlockSupport( blockType, 'lightBlockWrapper' );
const hasWideBlockSupport = hasBlockSupport( blockType, 'alignWide', true );
const isAlignValid = includes(
// Compute valid alignments without taking into account,
Expand Down
62 changes: 62 additions & 0 deletions packages/block-editor/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,3 +56,65 @@
@import "./components/editor-skeleton/style.scss";
@import "./components/inserter/style.scss";


// Default styles for alignments in case the theme doesn't provide them.
.wp-block {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, this should be something that a theme author can copy/paste to its stylesheet and tweak the numbers.
There are some tweaks necessary to achieve it though.

  • We should replace the data-align with the classNames.
  • We should see whether we can remove the wp-block className in favor of something closer to what themes do. Something more like .entry-content > *

When we'll be able to remove IE11 support, we could use CSS variables for the widths and margins.

Copy link
Member

Choose a reason for hiding this comment

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

  • We should see whether we can remove the wp-block className in favor of something closer to what themes do. Something more like .entry-content > *

If we're refactoring now anyway, I really think this is something that we should do now. I see the alignment wrapper as context to a block or blocks, not as a block itself.

Copy link
Member

Choose a reason for hiding this comment

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

Also, if this is the main reason for magically adding the alignment wrapper, and we're going to remove the wp-block class anyway, does it still make sense to magically add it? Tbh, I'd much rather have an explicit component for it instead of magically adding wrappers

Copy link
Member

Choose a reason for hiding this comment

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

My original ideal in #20650 (comment) was to have something like:

<AlignmentWrapper align={align}>
  <Block.figure />
</AlignmentWrapper>

Both in the edit and save function.

max-width: $content-width;
margin-left: auto;
margin-right: auto;
margin-top: $default-block-margin;
margin-bottom: $default-block-margin;

// Floated blocks shouldn't have margins
&[data-align="left"],
&[data-align="right"] {
margin-top: 0;
margin-bottom: 0;

& > * {
// z-index should be higher than regular blocks to allow selection on click
z-index: z-index(".wp-block {aligned left or right}");
}
}

// Left.
&[data-align="left"] > * {
// This is in the editor only; the image should be floated on the frontend.
/*!rtl:begin:ignore*/
float: left;
margin-right: 2em;
/*!rtl:end:ignore*/
}

// Right.
&[data-align="right"] > * {
// Right: This is in the editor only; the image should be floated on the frontend.
/*!rtl:begin:ignore*/
float: right;
margin-left: 2em;
/*!rtl:end:ignore*/
}

&[data-align="full"] {
max-width: none;
clear: both;

// Leave room for the UI
margin-left: -$block-padding;
margin-right: -$block-padding;
@include break-small() {
margin-left: -$block-padding - $block-padding - $block-side-ui-width - $border-width - $border-width;
margin-right: -$block-padding - $block-padding - $block-side-ui-width - $border-width - $border-width;
}
}

&[data-align="wide"] {
max-width: 1100px;
clear: both;
}

&[data-align="center"] {
display: flex;
justify-content: center;
}
}
7 changes: 0 additions & 7 deletions packages/block-library/src/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,6 @@
* This allows us to create normalization styles that are easily overridden by editor styles.
*/

// Provide every block with a default base margin. This margin provides a consistent spacing
// between blocks in the editor.
.editor-styles-wrapper .block-editor-block-list__block {
margin-top: $default-block-margin;
margin-bottom: $default-block-margin;
Copy link
Member

Choose a reason for hiding this comment

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

I've been wanting to remove this for ages, but there's always been some reason for keeping it. Can it safely be removed now? Cc @jasmussen

Copy link
Contributor

Choose a reason for hiding this comment

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

It seeems like we can! From quick testing in a theme that doesn't style the editor, things look right. And it'd definitely a rule we want to remove. So it does seem now's the time!

Note that these two rules seem to mostly take its place:

Screenshot 2020-03-09 at 13 55 09

and

Screenshot 2020-03-09 at 13 57 06

Good to be mindful of these removals, when we get to the DOM "lightening", can check on a per-block basis, because now it will be up to each individual block to ensure a margin.

}

// This tag marks the end of the styles that apply to editing canvas contents and need to be manipulated when we resize the editor.
#end-resizable-editor-section {
display: none;
Expand Down
Loading