Block API: Add supports object to block attributes to configure copy support
#38643
Conversation
stacimc
requested review from
adamziel,
draganescu,
ellatrix,
kevin940726,
noisysocks,
talldan and
tellthemachines
as code owners
stacimc
added
[Feature] Block API
stacimc
changed the title
supports object to block attributes to configure copy support
|
Size Change: +10.2 kB (+1%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
|
Relative to the implementation proposed in #34750, these are the changes, (thanks @noisysocks 😄):
|
| @@ -33,7 +33,10 @@ const POPOVER_PROPS = { | |||
| }; | |||
|
|
|||
| function CopyMenuItem( { blocks, onCopy } ) { | |||
| const ref = useCopyToClipboard( () => serialize( blocks ), onCopy ); | |||
| const ref = useCopyToClipboard( | |||
| () => serialize( blocks, { retainCopyAttributes: false } ), | |||
There was a problem hiding this comment.
retainCopyAttributes isn't a spectacular name, but I had difficulty coming up with a better one. excludeNonCopyableAttributes? retainOnlyCopyableAttributes?
There was a problem hiding this comment.
Maybe includeCopyableAttributes? Also let's add __experimental for the moment 😀
There was a problem hiding this comment.
Agreed about __experimental :)
Updated to __experimentalExcludeNonCopyableAttributes (and inverted the logic). It's an... intense name 😅 but I'm having trouble being more concise. I'm worried that something like retainCopyAttributes or includeCopyableAttributes doesn't really get across that it means retain only copyable attributes.
| __experimentalSanitizeBlockAttributes( block, attributes ); | ||
| // The __internalWidgetId is only defined on the client-side and must be removed before making the | ||
| // request. | ||
| const retainedAttributes = omit( attributes, [ '__internalWidgetId' ] ); | ||
|
|
There was a problem hiding this comment.
We might be able to do this in a more generic way, but the __internalWidgetId is actually a special case and is the only attribute that needs to be stripped here 🤔
My plan was to just add back in __experimentalSanitizeBlockAttributes, which on trunk handles the removal of the widgetId -- but that will no longer work! That function removes un-registered attributes, but the widgetId is registered now (just only on the client side).
We don't want to omit all attributes with copy: false here, because it's entirely possible to add 'normal' server-side attributes that don't support copy, and those shouldn't be stripped. Only the widgetId is a special case.
Alternative suggestions from @noisysocks on #34750:
We could just set 'additionalProperties' to true on the server. I don't really see the need to be so strict there. This might require a Core patch, though, as I'm not sure that it's possible for a plugin to re-register a route.
Or, we could have ServerSideRender perform a GET on the /block-types endpoint before making its request to /block-renderer. If an attribute isn't in the attributes object that comes back from /block-types, then it hasn't been defined on the server, and so we must not send it to /block-renderer.
There was a problem hiding this comment.
We could just set 'additionalProperties' to true on the server. I don't really see the need to be so strict there. This might require a Core patch, though, as I'm not sure that it's possible for a plugin to re-register a route.
I'm leaning towards this since it matches how the frontend works, i.e. you can put whatever you like in the block's markup and the block editor will silently ignore it.
But yeah this is fine as a temporary solution. Before merging, could you please create an issue to track this and link to it in a // TODO?
|
I much prefer this 😀 it's "KISS" and we're not overloading the meaning of |
|
I think this is better than |
|
Updated to change |
| const serialized = serialize( blocks ); | ||
| const serialized = serialize( blocks, { | ||
| __experimentalExcludeNonCopyableAttributes: | ||
| event.type === 'copy', |
There was a problem hiding this comment.
this one feels overly specific when we're introducing a general way we could accomplish this too.
const serializeOptiosn = event.type === 'copy'
? { __experimentalIncludeAttributes: [ 'copy' ]
: undefined;
const serialized = serialize( blocks, serializeOptions );Also I'm really lost on the double negative. A positive phrasing could make that easier to understand.
There was a problem hiding this comment.
(this would give us the ability to denote multiple sets of attributes for inclusion or sets for explicit exclusion)
{
__experimentalExcludeAttributes: [ 'style' ]
}There was a problem hiding this comment.
The variable name definitely leaves a lot to be desired, suggestions are very much appreciated 😄
Making it extensible to other sets of attributes is great! It gets a little busy if we want to include the ability to filter out by any attribute property (rather than limiting it to a list of supports keys for example):
const serializationOptions = {
__experimentalIncludeAttributes: {
__experimentalSupports: { copy: false },
}
}
const serialized = serialize(
blocks,
event.type === 'copy' ? serializationOptions : undefined
);
Not sure much can be done about that 🤔 I think the benefit to extensibility and the clearer naming is a big win.
There was a problem hiding this comment.
just a note about my example: I wasn't proposing nesting supports inside of includeAttributes. using your object notation instead of an array it could be this too, which is closer to what I was trying to imply. the include/exclude arrays have some advantages, but they aren't directly necessary here either
const options = {
__experimentalAttributeSupports: {
copy: false,
}
}There was a problem hiding this comment.
This is tricky. Nesting supports inside of includeAttributes is very verbose but reads clearly to me. I'm a little concerned with this:
// Is it clear from this that attributes that do not support `copy` will be omitted?
cloneBlock(
block,
__experimentalOptions: {
__experimentalSupports: { copy: false }
}
}
// Would it be easy to confuse with this -- here attributes that DO support `copy` would be omitted:
cloneBlock(
block,
__experimentalOptions: {
__experimentalSupports: { copy: true }
}
}
Sorry if I'm misunderstanding, the double negatives are confusing.
// This is more 'streamlined' and reads more clearly to me, but only supports excluding attributes by `supports` keys. Eg you can't exclude attributes by their role for example
cloneBlock(
block,
__experimentalOptions: {
__experimentalExcludeAttributes: { copy: false }
}
}
| @@ -1172,7 +1171,7 @@ export const duplicateBlocks = ( clientIds, updateSelection = true ) => ( { | |||
| last( castArray( clientIds ) ) | |||
| ); | |||
| const clonedBlocks = blocks.map( ( block ) => | |||
| __experimentalCloneSanitizedBlock( block ) | |||
| cloneBlock( block, {}, null, { retainCopyAttributes: false } ) | |||
There was a problem hiding this comment.
wouldn't cloning imply retaining?
same question as above with regards to being overly-specific to copy
There was a problem hiding this comment.
I missed updating retainCopyAttributes here, I'll update that when I settle on the API.
wouldn't cloning imply retaining?
In the driving use cases of a productId or widgetId, we don't want it retained on copy (whether that be from the block menu or keyboard commands) or on duplication. With this API though we could consider those to be separate supports:
"__experimentalSupports": {
"copy": false,
"duplicate": false
}
In either case, I think it's a good point that adding the logic to cloneBlock is unnecessarily confusing -- we could clone the block and strip the attributes afterward, here in the duplication logic. In an earlier iteration it made a little more sense to add the logic to cloneBlock because we were using it in multiple places.
| saveAttributes = omit( | ||
| saveAttributes, | ||
| __experimentalFilterBlockAttributes( blockName, { | ||
| __experimentalSupports: { copy: false }, |
There was a problem hiding this comment.
this here feels like it could be the thing exposed through serializeBlock's options. do we need both the inner and outer language of talking about it?
|
@stacimc thank you for all your work and the patience with all the back and forth in the approach 😄 Is the PR ready for review? I've started testing(without looking at the code yet) but I'm not observing the expected results.. I've tried adding to |
stacimc
force-pushed
the
try/copy-support-for-block-attributes
branch
from
1e20f1d to
6afe9e0
Compare
Rebased -- and fixed a bug with |
Thanks so much for doing this! I'll test and review tomorrow! |
stacimc
force-pushed
the
try/copy-support-for-block-attributes
branch
from
5e5ca19 to
94efe0c
Compare
|
Rebased.
@dmsnell The initial version was something close to that! One concern I have with the word I think part of the problem is that the object passed in is actually a filter which matches the structure of the attribute's configuration in What do you think about |
|
There are a few ways we can address the language, some of which none of us have considered yet. For one, we don't have to pass an object. Do we ever have a valid case to call excludeAttributes: [ 'copy' ]For two I'm not sure that
the word |
|
The reason I said I've also considered just passing a boolean, something like: |
|
Is there a problem with leaving the abstraction one-level deep and exposing cloneBlock( block, {}, null, { __experimentalSupports: { copy: false } } );In terms of cloning that seems like there's a reasonable semantic connection between attributes we carry over and those we leave behind, plus it eliminates the confusion that |
My worry is it conflates the concepts of what the clone operation supports (what it looks like) versus what the attributes it includes and excludes support (what it's actually doing). To clarify: You could interpret this to say the clone operation itself should not support Or to put it another way, let's say we add additional support keys for attributes, and I would like to clone a block and exclude attributes that do support But that does the exact opposite of what I want, it excludes attributes that have Maybe that latter case is contrived, but I think it's semantically clearer to be fully explicit, as heavy as it is: |
|
I pushed this update: For the reasons I mentioned here. I think this is at least clear, although I acknowledge it's heavy-handed. I think we could go in circles on this one, so I'm perfectly happy to change the language if folks generally prefer a simpler option. Given that this is only being added to |
|
It sure is a mess 😄 but maybe we'll eventually think of something without the confusion. This feels like a direct clone of the problem with One last check: was there a problem with the array-of-supports-names? I still find that at least easier since it's presence-or-absence instead of a boolean. Did we consider |
Ha, definitely! Thank you for sticking with me 😅
I think it would have to be even slightly longer! Or we could take your earlier suggestion to invert the logic and adjust it to |
as long as that is I feel like it's the clearest yet. it even reveals how deep the double-negations go, as I think I'm still getting lost knowing what the intentions are when I read these.
|
Agreed 😄 Random inspiration: how about something like |
I like this, but with one possible modification that we link it back to attributes since otherwise it's only evident to you and me that's the case. blocks, which are cloned, also have
|
Closes #29693
Alternative to #32604, #34750
Description
This PR:
supportsobject property to block attributes. It includes thecopysupport, used to indicate whether the attribute should be copied on block duplication. It is true by default.Example fragment of block.json:
Copy support is true by default, meaning it is true if the
supports.copyis not configured, and if thesupportsproperty is missing altogether.__internalWidgetIDto use this new property/For just one example of how this might be used: the Jetpack
Pay with Paypalblock uses aproductIdattribute to identify the actual product record referenced by the block. This data is meant to be internal to the block and should not be exposed to the user. When the user duplicates the block, we want to copy the other attributes used to store price info and other details, but create a brand new product record/productId.Notes
It respects any configured default valuesDefault values are not filled in on block duplication either; support for this could be easily added if a viable use case becomes apparent, thoughsetAttributescontentattribute of the Code block would not make sense, because although the attribute would not be copied on duplication, it will populate with the same value from the html.Attributes without copy support are not removed when you convert a block into a Reusable block. I don’t know a ton about Reusable blocks, so calling this out so that someone more familiar can correct me; my understanding is that multiple instances of a Reusable block are not independent of each other (ie editing one copy affects them all), so even 'internal' attributes should also be shared.
How has this been tested?
supports: { copy: false }to any block attribute.Test with attributes that have adefaultvalue and ensure that the default is respected in the duplicated blockCopymenu item, and by keyboard commandsScreenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).