-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
Gallery, Image: Move attributes to block boundary (JSON) #11212
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While these changes seem valid and right now they look like the most promising way to have access to this attributes on the server, they raise some questions.
If for an image we expose the URL, alt, etc as attributes that server-side code can access aren't we creating the expectation that accessing the video or an audio URL is possible? In what cases it is ok to source the attribute from the HTML markup and in what cases the attribute should be exposed?
I think the reason for the CI failure is that we need to update the demo content in https://github.com/WordPress/gutenberg/blob/update/gallery-images-attr-source/post-content.php. I think we may also need to update some snapshots.
const blockAttributes = { | ||
images: { | ||
type: 'array', | ||
default: [], | ||
source: 'query', | ||
selector: 'ul.wp-block-gallery .blocks-gallery-item', | ||
query: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove all this query data specifying each property of the object? It looks like it is not used at all now (because the source is not a query). If we keep them it may give the idea that it is some "way" to validate the properties of the image object, but that is not the case. If the object contains other properties they are passed to the block anyway.
I absolutely agree with you. I'm not happy about these changes. Part of the challenge is assessing how important #8193 is that we accept this compromise. I'll add a few labels for this discussion. |
There's also some related discussion in #10444. I feel like in almost every case, you should be able to recreate a block solely from its attributes. Having to parse values from HTML markup makes it harder for a block's structure to change over time, makes server side overriding of a block's rendered output effectively impossible, limits the ability to build blocks from an API endpoint (e.g. consuming a JSON representation of a block's structure and rendering it to native view code in an iOS or Android app), etc. I don't know how that necessarily jives with the high level technical architecture goals, but inferring user-configurable settings from HTML feels like the hack. If you're doing that, why have attributes at all? (Okay that's obviously a bit hyperbolic, but you get my gist 😅) EDIT: Obviously when you get into inner content/RichText content things get a little muddier, but these cases (and the examples of the video/audio blocks too) feel like easy wins. |
If hypothetically we had a server-side API that could get us a block's full attribute set — as opposed to just what is in the comment JSON — and we had blocks systematically registered on the server (even when they are simple static blocks), what issues would still be standing? $block_type_settings = array(
'attributes' => array(
'foo' => array( 'selector' => 'cite', … ),
),
'deprecated' => array(
array(
'attributes' => array(
'foo' => array( 'selector' => 'footer', … ),
)
),
),
) In my mind, this solves:
Am I missing some perspective, @chrisvanpatten? I know that it's an issue that we won't have the needed tooling in time for 5.0, but I'm still convinced that once we have it we'll be able to handle a number of related problems. |
@mcsf that does technically check all the boxes but I’m also not really sure why that’s any better than setting specific attributes. Maybe it’s better to approach from the other side: why would you not want to set a specific attribute and instead pull a value from HTML? What are the advantages? (To me, being explicit and storing data as a block attribute makes it easier to also define attribute types, build server side validation, etc. Embedding within the block just means you have to trust parsers, trust that block HTML won’t get mangled, etc.) |
To add a bit There was some related recent discussion here: #1450 In the case of gallery we do not store ‘ids’ in an easy to consume way. I made a shortcode “classic” block that depends on a list of ids. Having them stored makes it possible to transform into the fallback block and maybe others |
Closing in favour of #11540. |
Description
Fixes #8193. See issue for context. In short: in order to allow easy server-side inspection and interception of Gallery and Image blocks, move away from sourced attributes — that would require server-side HTML-parsing tools that we don't yet have — towards "plain" attributes that are duplicated in the HTML comments of blocks.
How has this been tested?
A Gallery is henceforth saved as:
and an Image:
Checklist: