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

Buttons: Preserve styling from adjacent button blocks when inserting a new button block #37905

Merged
merged 12 commits into from
Jan 20, 2022
Merged

Buttons: Preserve styling from adjacent button blocks when inserting a new button block #37905

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
85aff5e
Buttons: Attempt to grab attributes from existing button blocks when …
andrewserong Jan 12, 2022
d9204b4
Move logic to get attributes from adjacent block to the inserter
andrewserong Jan 13, 2022
ed10faa
Clean up unneeded changes
andrewserong Jan 13, 2022
090aee8
Fix regression with Navigation blocks
andrewserong Jan 14, 2022
44538f6
Add extra inline comment about setting default attributes
andrewserong Jan 14, 2022
8d04641
Refactor the directInsertBlock to be an object instead of an array, a…
andrewserong Jan 17, 2022
a32fa27
Remove innerBlocks from createBlock function call
andrewserong Jan 17, 2022
392b83e
Allow directInsertBlock attributes to override adjacent block attributes
andrewserong Jan 17, 2022
f21abdf
Fix regression with navigation block inserter
andrewserong Jan 18, 2022
af31fee
Swap order of attributes for createBlock, add extra Button attributes…
andrewserong Jan 19, 2022
8f7dc06
Move logic to copy attributes to the getAdjacentBlockAttributes function
andrewserong Jan 19, 2022
401358f
Switch attributesToCopy to be an array of strings
andrewserong Jan 20, 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
30 changes: 16 additions & 14 deletions packages/block-editor/src/components/inner-blocks/use-nested-settings-update.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,27 +11,29 @@ import isShallowEqual from '@wordpress/is-shallow-equal';
import { store as blockEditorStore } from '../../store';
import { getLayoutType } from '../../layouts';

/** @typedef {import('../../selectors').WPDirectInsertBlock } WPDirectInsertBlock */

/**
* This hook is a side effect which updates the block-editor store when changes
* happen to inner block settings. The given props are transformed into a
* settings object, and if that is different from the current settings object in
* the block-editor store, then the store is updated with the new settings which
* came from props.
*
* @param {string} clientId The client ID of the block to update.
* @param {string[]} allowedBlocks An array of block names which are permitted
* in inner blocks.
* @param {?Array} __experimentalDefaultBlock The default block to insert: [ blockName, { blockAttributes } ].
* @param {?Function|boolean} __experimentalDirectInsert If a default block should be inserted directly by the
* appender.
* @param {string} [templateLock] The template lock specified for the inner
* blocks component. (e.g. "all")
* @param {boolean} captureToolbars Whether or children toolbars should be shown
* in the inner blocks component rather than on
* the child block.
* @param {string} orientation The direction in which the block
* should face.
* @param {Object} layout The layout object for the block container.
* @param {string} clientId The client ID of the block to update.
* @param {string[]} allowedBlocks An array of block names which are permitted
* in inner blocks.
* @param {?WPDirectInsertBlock} __experimentalDefaultBlock The default block to insert: [ blockName, { blockAttributes } ].
* @param {?Function|boolean} __experimentalDirectInsert If a default block should be inserted directly by the
* appender.
* @param {string} [templateLock] The template lock specified for the inner
* blocks component. (e.g. "all")
* @param {boolean} captureToolbars Whether or children toolbars should be shown
* in the inner blocks component rather than on
* the child block.
* @param {string} orientation The direction in which the block
* should face.
* @param {Object} layout The layout object for the block container.
*/
export default function useNestedSettingsUpdate(
clientId,
Expand Down
82 changes: 77 additions & 5 deletions packages/block-editor/src/components/inserter/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ class Inserter extends Component {
onSelectOrClose,
} = this.props;

if ( hasSingleBlockType || directInsertBlock?.length ) {
if ( hasSingleBlockType || directInsertBlock ) {
return this.renderToggle( { onToggle: insertOnlyAllowedBlock } );
}

Expand Down Expand Up @@ -251,10 +251,54 @@ export default compose( [
onSelectOrClose,
} = ownProps;

if ( ! hasSingleBlockType && ! directInsertBlock?.length ) {
if ( ! hasSingleBlockType && ! directInsertBlock ) {
return;
}

function getAdjacentBlockAttributes() {
const { getBlock, getPreviousBlockClientId } = select(
blockEditorStore
);

if ( ! clientId && ! rootClientId ) {
return {};
}

// If there is no clientId, then attempt to get attributes
// from the last block within innerBlocks of the root block.
if ( ! clientId ) {
const parentBlock = getBlock( rootClientId );

if ( parentBlock?.innerBlocks?.length ) {
const lastInnerBlock =
parentBlock.innerBlocks[
parentBlock.innerBlocks.length - 1
];

if (
directInsertBlock &&
directInsertBlock?.name === lastInnerBlock.name
) {
return { ...lastInnerBlock.attributes };
}
}
return {};
}

// Attempt to get attributes from the previous block
// relative to the current clientId.
const currentBlock = getBlock( clientId );
const previousBlock = getBlock(
getPreviousBlockClientId( clientId )
);

if ( currentBlock?.name === previousBlock?.name ) {
return { ...( previousBlock?.attributes || {} ) };
}

return {};
}

function getInsertionIndex() {
const {
getBlockIndex,
Expand Down Expand Up @@ -284,9 +328,37 @@ export default compose( [

const { insertBlock } = dispatch( blockEditorStore );

const blockToInsert = directInsertBlock?.length
? createBlock( ...directInsertBlock )
: createBlock( allowedBlockType.name );
let blockToInsert;

// Attempt to augment the directInsertBlock with attributes from an adjacent block.
// This ensures that styling from existing nearby blocks are preserved in the
// newly inserted block. To support intentionally clearing out certain attributes,
// the attributes of the directInsertBlock override those of the adjacent block.
// See: https://github.com/WordPress/gutenberg/issues/37904
if ( directInsertBlock ) {
const newAttributes = {};
const adjacentBlockAttributes = getAdjacentBlockAttributes();

Object.keys( adjacentBlockAttributes ).forEach(
( attributeName ) => {
if (
directInsertBlock.attributesToCopy?.[
attributeName
]
) {
newAttributes[ attributeName ] =
adjacentBlockAttributes[ attributeName ];
}
}
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this will cycle through all the adjacent block attributes even if we don't mean to copy any of them. I'm thinking it would be better to return early if there are no attributes to be copied. A couple ways we could do this:

  • Wrap this logic inside another if statement checking for directInsertBlock.attributesToCopy; or
  • pass the attributes to copy to getAdjacentBlockAttributes and have it return only those attributes; in that case we can return early from that function if nothing is passed to it. I'm more inclined towards this option as it would tidy all the extra attribute logic inside getAdjacentBlockAttributes. What do you think?

Copy link
Contributor Author

@andrewserong andrewserong Jan 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Great points! I've moved the logic up to go inside getAdjacentBlockAttributes. That function is a little long now, but it feels a bit cleaner putting it there, and also only iterating over the specified attributesToCopy if provided rather than every attribute of the adjacent block 👍

Let me know if you think it needs further tidying (or refactoring to another file now that it's a bit longer!)


blockToInsert = createBlock( directInsertBlock.name, {
...newAttributes,
...( directInsertBlock.attributes || {} ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my curiosity and to see if I missed a test case, did you have a use case in mind where the directInsertBlock would override attributes of the adjacent block?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. directInsertBlock could have attributes of its own defined; in case we define an attribute and also want to grab the same attribute from an adjacent block, it might make more sense for the adjacent one to override the initial.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, thanks for asking the question! I wasn't too sure of a use case, but thought it'd still be good to allow attributes to be defined in the directInsertBlock. Allowing the adjacent ones to override sounds good to me, I've swapped the order here 👍

} );
} else {
blockToInsert = createBlock( allowedBlockType.name );
}

insertBlock( blockToInsert, getInsertionIndex(), rootClientId );

Expand Down
11 changes: 8 additions & 3 deletions packages/block-editor/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1844,10 +1844,15 @@ export const __experimentalGetAllowedBlocks = createSelector(
/**
* Returns the block to be directly inserted by the block appender.
*
* @param {Object} state Editor state.
* @param {?string} rootClientId Optional root client ID of block list.
* @param {Object} state Editor state.
* @param {?string} rootClientId Optional root client ID of block list.
*
* @return {?WPDirectInsertBlock} The block type to be directly inserted.
*
* @return {?Array} The block type to be directly inserted.
* @typedef {Object} WPDirectInsertBlock
* @property {string} name The type of block.
* @property {?Object} attributes Attributes to pass to the newly created block.
* @property {?Object} attributesToCopy Attributes to be copied from adjecent blocks when inserted.
*/
export const __experimentalGetDirectInsertBlock = createSelector(
( state, rootClientId = null ) => {
Expand Down
13 changes: 13 additions & 0 deletions packages/block-library/src/buttons/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,17 @@ import { name as buttonBlockName } from '../button';

const ALLOWED_BLOCKS = [ buttonBlockName ];

const DEFAULT_BLOCK = {
name: buttonBlockName,
attributesToCopy: {
backgroundColor: true,
className: true,
gradient: true,
textColor: true,
width: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also want to copy border and spacing 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.

Oh, yes, and font settings, too! I totally forgot about those attributes provided by block supports. I've added a few more here and it seems to be working well:

Kapture 2022-01-19 at 16 52 11

},
};

function ButtonsEdit( { attributes: { layout = {} } } ) {
const blockProps = useBlockProps();
const preferredStyle = useSelect( ( select ) => {
Expand All @@ -26,6 +37,8 @@ function ButtonsEdit( { attributes: { layout = {} } } ) {

const innerBlocksProps = useInnerBlocksProps( blockProps, {
allowedBlocks: ALLOWED_BLOCKS,
__experimentalDefaultBlock: DEFAULT_BLOCK,
__experimentalDirectInsert: true,
template: [
[
buttonBlockName,
Expand Down
4 changes: 3 additions & 1 deletion packages/block-library/src/navigation-submenu/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,9 @@ import { name } from './block.json';

const ALLOWED_BLOCKS = [ 'core/navigation-link', 'core/navigation-submenu' ];

const DEFAULT_BLOCK = [ 'core/navigation-link' ];
const DEFAULT_BLOCK = {
name: 'core/navigation-link',
};

const MAX_NESTING = 5;

Expand Down
4 changes: 3 additions & 1 deletion packages/block-library/src/navigation/edit/inner-blocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ const ALLOWED_BLOCKS = [
'core/navigation-submenu',
];

const DEFAULT_BLOCK = [ 'core/navigation-link' ];
const DEFAULT_BLOCK = {
name: 'core/navigation-link',
};

const LAYOUT = {
type: 'default',
Expand Down