Move the directives and markup needed for the lightbox to block supports #51232
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SantosGuillamot
added
the
[Feature] Interactivity API
|
Size Change: +184 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
artemiomorales
approved these changes
Jun 5, 2023
There was a problem hiding this comment.
Looks good to me!
EDIT: Realized I shouldn't have approved these changes yet, as we need to make sure it's synced with the latest code from #51206
SantosGuillamot
requested review from
spacedmonkey,
ellatrix,
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
Flaky tests detected in d29fc03. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5188800288
|
cbravobernal
approved these changes
Jun 6, 2023
ajlende
reviewed
Jun 6, 2023
There was a problem hiding this comment.
A couple small nits, but otherwise this is looking good. I'm happy the duotone support works for it without much hassle this way.
Also, I found another issue (#51276) while testing, but I think it's unrelated to this change.
lib/block-supports/behaviors.php
Outdated
| * @param array $block Block object. | ||
| * @return string Filtered block content. | ||
| */ | ||
| function gutenberg_blocks_supports_add_lightbox( $block_content, $block ) { |
There was a problem hiding this comment.
Small nit. All of the other block supports that update rendering follow the same naming pattern which makes it a little easier to search through.
gutenberg_render_behaviors_support would match the pattern here.
There was a problem hiding this comment.
Oh, great! I will make that change as well 🙂
lib/block-supports/behaviors.php
Outdated
| // In the future, this should be a loop with all the behaviors. | ||
| $has_lightbox_support = block_has_support( $block_type, array( 'behaviors', 'lightbox' ), false ); | ||
| if ( $has_lightbox_support ) { | ||
| add_filter( 'render_block_' . $block_type->name, 'gutenberg_blocks_supports_add_lightbox', 15, 2 ); |
There was a problem hiding this comment.
Why are we using priority 15 instead of 10? We probably want to add a comment explaining that.
I can see that we'd want it to run later than the other block supports, but I figured that's why you were using the render_block_{block_name} instead of render_block filter.
There was a problem hiding this comment.
I thought that there could be external plugins using the render_block_{block_name} filter to modify the image as the duotone is doing, and I believe this hook should run after the default ones.
I'll add a comment for that. Thank you! 🙂
|
Merging this pull request. As the functionality is still experimental, we can still change it in the future. |
ockham
reviewed
Jun 15, 2023
Comment on lines
+32
to
+34
| // Use priority 15 to run this hook after other hooks/plugins. | ||
| // They could use the `render_block_{$this->name}` filter to modify the markup. | ||
| add_filter( 'render_block_' . $block_type->name, 'gutenberg_render_behaviors_support_lightbox', 15, 2 ); |
There was a problem hiding this comment.
Bit late to this, but I think there's a dedicated apply argument to WP_Block_Supports::get_instance()->register() that's typically used to render the block-supports on the frontend based on the attribute(s) registered (see e.g. how thealign block-support does it).
Maybe that would be a better fit here rather than using the (more generic) render_block_{$type} filter?
There was a problem hiding this comment.
We won't be landing this into 6.3, so I guess is not to late to update it (or make another PR to update it).
There was a problem hiding this comment.
We can definitely explore that option deeper. I think we tried, but we didn't fully understand how that worked, so we ended up using a filter like the duotone. We need to run this code after other filters, like the duotone, run, so we thought using another filter with lower priority was the proper way. But I'm happy to explore other possibilities if you think that's possible.
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
…rts (WordPress#51232) * Move behaviors to block supports * Remove apply from block supports and format PHP * Update core-blocks reference docs * Update latests changes to the image lightbox * Add comment to explain the priority * Change function name to match patterns
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Marking this as a draft because the code will need to be updated with the changes made in this other pull request.
What?
Instead of having the directives and the markup needed to make the lightbox behavior work in the Image block
render_callback, this pull request moves them to arender_blockfilter called in the block supports.Why?
There are mainly two reasons for this pull request:
render_blockfilter (like the duotone is doing adding a class), those changes are not applied to the markup. It would solve this bug.Note that this pull request doesn't aim to completely abstract the lightbox behavior, it would be just a good first step. There are some things missing in that regard that I believe could be handled in a subsequent pull request:
How?
render_callbackof the image block.supports.behaviors.lightboxusingWP_Block_Supports::get_instance()->register.render_block_{$this->name}filter to add the directives and the markup.Testing Instructions
Video explanation
https://www.loom.com/share/09ffb096070943d7b73ed0d885c40fd4