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

Changes from 1 commit
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
65 changes: 34 additions & 31 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 @@ -255,15 +255,21 @@ export default compose( [
return;
}

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

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

const result = {};
let adjacentAttributes = {};

// If there is no clientId, then attempt to get attributes
// from the last block within innerBlocks of the root block.
if ( ! clientId ) {
Expand All @@ -279,24 +285,35 @@ export default compose( [
directInsertBlock &&
directInsertBlock?.name === lastInnerBlock.name
) {
return { ...lastInnerBlock.attributes };
adjacentAttributes = lastInnerBlock.attributes;
}
}
return {};
} else {
// Otherwise, 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 ) {
adjacentAttributes =
previousBlock?.attributes || {};
}
}

// 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 || {} ) };
// Copy over only those attributes flagged to be copied.
for ( const attribute in attributesToCopy ) {
if (
attributesToCopy[ attribute ] &&
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 remove this and just check:

if ( adjacentAttributes.hasOwnProperty( attribute ) )?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for re-testing @stacimc, another good question! I wasn't too sure about this one — the attributesToCopy is currently an object where each attribute is set to a boolean (true in all the cases in the Buttons block). So, if someone were to theoretically set one of them to false it might be odd if it still copied the attribute.

I initially had it an object so that look up was easy when looping over the adjacent block's attributes, so an object look up was preferable to an array. Now that it's switched the other way (we're looping over the attributesToCopy) the object structure doesn't seem to be quite as necessary. I kinda like the parallel, though, with having both attributes and attributesToCopy being objects.

So, a couple of follow-up questions! 😀

Is it better for attributesToCopy to be an object or an array? And if we keep it as an object, should it be possible for someone to set one of the values to false to switch off copying of that attribute (there's no real use case for this, but I think it determines whether or not we should remove this line 🤔)

Apologies if I'm overthinking this!

Copy link
Contributor

@stacimc stacimc Jan 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Oh of course, attributesToCopy is an object now 🤦‍♀️ You're right, this is a bit tricky! Provided attributesToCopy remains an object, I'd leave it as you have it. I don't think anyone ought to explicitly set a value to false within that object, but I think you're right to safeguard against it.

To me it does feel slightly less clear when reading -- maybe just a naming thing, but I wouldn't expect a parameter named attributesToCopy to contain data for attributes that shouldn't be copied. I may be the one overthinking it here, though 😄 I don't feel strongly about changing it and I don't think it should block the PR, so up to you!

Copy link
Contributor

Choose a reason for hiding this comment

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

Making attributesToCopy an array would make it less verbose to add those attributes, and semantically makes more sense because we wouldn't ever expect them to be set to false (there's no point adding an attribute to copy if we don't want it copied, and there's no way of toggling those attributes after they're set).

I agree it's not a blocking issue for this PR though; we can always address it in a follow up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @tellthemachines, that's a great point. I've updated it to be an array in 401358f (and updated the typedef so that it's an array of strings), and that looks much clearer to me. Cheers!

I'm out for the next few hours, but will 🤞 merge in this PR tonight once the tests pass, unless there are any objections 🙂

adjacentAttributes.hasOwnProperty( attribute )
) {
result[ attribute ] =
adjacentAttributes[ attribute ];
}
}

return {};
return result;
}

function getInsertionIndex() {
Expand Down Expand Up @@ -331,25 +348,11 @@ export default compose( [
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.
// This ensures styling from nearby blocks is preserved in the newly inserted 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 ];
}
}
const newAttributes = getAdjacentBlockAttributes(
directInsertBlock.attributesToCopy
);
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, {
Expand Down