Buttons: Preserve styling from adjacent button blocks when inserting a new button block #37905
Conversation
andrewserong
added
[Type] Enhancement
|
Size Change: +955 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
andrewserong
added
the
[Feature] Inserter
|
@tellthemachines I've added you as a reviewer since you worked on adding the |
There was a problem hiding this comment.
This would be such a welcome change! IMO it dramatically improves the experience of working with the Buttons block. I'm interested to hear what others think, but I also like its addition to the defaultBlock logic and its potential to be used more universally. I appreciate there's already handling for inner blocks of different types :)
This tests great for me with the Buttons block! I've tried adding all possible attributes, and inserting buttons in different positions. Attributes are correctly taken from the previous block, and text/url are not copied. This feels so much more intuitive ✨
button-inserter.mov
I just caught one regression with the Navigation block. When you add a submenu and attempt to insert links, you'll get the inserter on the first link. If you try to add additional links, though, it just copies the first link over rather than opening the inserter. I think you'll just need to mark some link attributes like url as undefined so they don't get copied over -- looks like maybe in the navigation-submenu DEFAULT_BLOCK?
navigation-submenu.mov
Other than that, Navigation also looks good to me!
|
Thanks for reviewing @stacimc! Good catch with the Navigation and Navigation Submenu blocks — I think from the looks of it the Navigation Link block mostly has attributes that we wouldn't want to copy over when inserting a block. However, I quite like the default behaviour of copying across attributes, so in 29774c9 I've had a go at setting the Navigation Link's Happy for any feedback if folks think there's a better way to handle it (particularly from folks who've done more work on the Navigation block) — I was trying to avoid adding another flag to switch on the behaviour in this PR, but there might be a trade-off to be found if the approach of setting defaults is too unwieldy! Anyway, here's a screengrab of the navigation link insertion after the above commit was pushed (I think it's working properly now): Kapture.2022-01-14.at.12.11.08.mp4 |
| ...( directInsertBlock[ 1 ] || {} ), | ||
| }, | ||
| ] | ||
| : directInsertBlock; |
There was a problem hiding this comment.
This ternary is a little long for comfortable reading; could we turn it into an if else? Perhaps we can conflate it with the ternary below and just have something like
let blockToInsert
if ( directInsertBlock?.length )
blockToInsert = createBlock( // get attribs etc )
else
blockToInsert = createBlock( allowedBlockType.name )
There was a problem hiding this comment.
Thanks that's a good suggestion, I'll have a play with that. I struggled a little with this PR in trying to keep the logic neat (I mostly hacked it together and then tried in vain to get it presentable 😅), so I very much appreciate the extra set of 👀!
| } ), | ||
| {} | ||
| ), | ||
| }, |
There was a problem hiding this comment.
It's not great that we have to add exceptions to copied attributes in every single place we use direct insert. We may not be using it in a lot of core blocks (and there probably won't be many more), but if/when it becomes stable, we want third party block developers to find it easy to use.
One alternative would be to specify in DEFAULT_BLOCK a list of attributes we want copied, and have getAdjacentBlockAttributes only return those; then by default if nothing is specified nothing is copied.
Or we could add an __experimentalCopyStyleAttributes instead of leveraging direct insert, but what we're doing here is close enough to direct insert that it kind of makes sense to have it there.
There was a problem hiding this comment.
It's not great that we have to add exceptions to copied attributes in every single place we use direct insert.
Definitely not, thanks for taking a look here!
One alternative would be to specify in DEFAULT_BLOCK a list of attributes we want copied, and have getAdjacentBlockAttributes only return those; then by default if nothing is specified nothing is copied.
Oh, I like that idea — I felt a bit uneasy just copying everything, and I think it makes it cleaner if we can keep it a part of DEFAULT_BLOCK if possible without adding an extra flag.
At the moment DEFAULT_BLOCK is an array so that it matches the arguments we pass to createBlock. I was wondering if it'd make sense to switch it to an object, so that we could add an extra key for the attributes to be copied, and have something like the following?
const DEFAULT_BLOCK = {
name: 'core/my-block',
attributes: {
myDefaultAttribute: 'default value to set'
},
attributesToCopy: {
myCopiedAttribute: true
}
}
I'll keep chipping away at this PR next week, but let me know if you have a particular preference for how we implement it. Either way, explicitly flagging when we'd like to copy attributes sounds safer than assuming and having block authors have to add exceptions 🙂
|
@tellthemachines and @stacimc, I've had a go at refactoring this so that it's opt-in to copy over attributes from adjacent blocks rather than opt-out, so that 🤞 it's a little more straightforward for either existing blocks, or 3rd party block authors to work with (i.e. they don't need to know to exclude attributes from being copied). To get there, I've also refactored usage of the Based on a comment from @gwwar on the earlier PR that introduced the I'm pretty sure this PR is now ready for another review, but it's the end of my day now so I haven't tested it fully just yet. Will do some more testing tomorrow! |
…inserting a new button block
…dd attributesToCopy prop
andrewserong
force-pushed
the
update/buttons-block-inserter-button-to-inherit-styles
branch
from
294f18f
to
f21abdf
Compare
|
Alrighty, I think this PR is working correctly now (fixed a regression with the Navigation block inserter) 👍 |
There was a problem hiding this comment.
This is looking good to me! Copying nothing by default and requiring configuration of attributes to copy in the WPDirectInsertBlock seems very reasonable; it is more intentional and probably less likely to cause error if misconfigured.
I'll defer to those more familiar with the Navigation block and the directInsertBlock implementation, but the use of the object looks okay to me with the addition of the typedef. If I read the linked conversation correctly, there's no current use case for including the innerBlocks and it's safer to omit it for now, so I would hazard you've got the right idea there as well.
- Style attributes are copied from the previous button block
- Url & text attributes are not copied
- Works when inserting between two existing buttons, or at the end of the list
I re-tested Navigation as well:
- Links are added by default
- No attributes are copied over as new links are added
- Submenus can be added, and links are added to them by default
As soon as you add a non-link block to a Navigation menu (for example, if you add the Site Logo), clicking Insert will bring up the full block inserter menu. This is what's happening on trunk as well, and I believe that's the intended behavior.
|
|
||
| blockToInsert = createBlock( directInsertBlock.name, { | ||
| ...newAttributes, | ||
| ...( directInsertBlock.attributes || {} ), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
| adjacentBlockAttributes[ attributeName ]; | ||
| } | ||
| } | ||
| ); |
There was a problem hiding this comment.
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
getAdjacentBlockAttributesand 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 insidegetAdjacentBlockAttributes. What do you think?
There was a problem hiding this comment.
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 || {} ), |
There was a problem hiding this comment.
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.
| className: true, | ||
| gradient: true, | ||
| textColor: true, | ||
| width: true, |
There was a problem hiding this comment.
Do we also want to copy border and spacing styles?
There was a problem hiding this comment.
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:
|
Thanks again for the reviews @stacimc and @tellthemachines! I think I've implemented all the feedback, but let me know if I've missed anything 😀 |
| // Copy over only those attributes flagged to be copied. | ||
| for ( const attribute in attributesToCopy ) { | ||
| if ( | ||
| attributesToCopy[ attribute ] && |
There was a problem hiding this comment.
Can we remove this and just check:
if ( adjacentAttributes.hasOwnProperty( attribute ) )?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
+1 for moving the logic into getAdjacentAttributes, which also has the advantage of making getInsertionIndex more readable.
I neglected to test all attributes on my second run-through, sorry! Tested again and all style attributes are being copied over, including attrs from block supports and additional CSS classes 😄
Just one question, but LGTM!
| // Copy over only those attributes flagged to be copied. | ||
| for ( const attribute in attributesToCopy ) { | ||
| if ( | ||
| attributesToCopy[ attribute ] && |
There was a problem hiding this comment.
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.
andrewserong
changed the title
andrewserong
deleted the
update/buttons-block-inserter-button-to-inherit-styles
branch
Description
Fixes #37904
This PR updates the Inserter logic for the
defaultBlockto attempt to use attributes from an adjacent similar block when inserting a new block directly between two blocks. The use case is primarily for the Buttons block, where clicking between two individual Button blocks inserts a new button. For layouts or patterns that use full-width buttons (e.g. a Link in Bio pattern), or buttons with particular styling, it might make it smoother for users if in the inserted button we use those existing attributes.I'd love feedback on whether or not this is universal enough to be added to the
defaultBlocklogic added by @tellthemachines in #34899.The logic in this PR includes:
directInsertBlockhas anattributesToCopyarray with matching attribute names.directInsertBlockto be an object instead of an array (to allow for theattributesToCopyprop) and added a typedefFor a precedent surrounding the suggested behaviour when inserting the button block, currently if you hit "enter" on a button block to fire the
onSplitfunction, it uses the existing attributes when creating the new button block.How has this been tested?
__experimentalDefaultBlockoption when registering the block (e.g. Navigation and Navigation Submenu blocks)Screenshots
Kapture.2022-01-13.at.17.21.59.mp4
Types of changes
Somewhere between a bug fix and an enhancement (I'd argue it's an enhancement but it will likely feel like a bug fix to some users)
Checklist:
*.native.jsfiles for terms that need renaming or removal).