close

Make WordPress Core

Opened 8 weeks ago

Closed 7 weeks ago

#64624 closed defect (bug) (fixed)

Fix grid layout for style variations defining blockGap

Reported by: isabel_brison's profile isabel_brison Owned by: isabel_brison's profile isabel_brison
Milestone: 7.0 Priority: normal
Severity: normal Version:
Component: Editor Keywords: gutenberg-merge has-patch has-unit-tests
Focuses: Cc:

Description

Tracking PHP changes for this Gutenberg PR: https://github.com/WordPress/gutenberg/pull/75360

Change History (10)

Image

This ticket was mentioned in PR #10906 on WordPress/wordpress-develop by @isabel_brison.


8 weeks ago
#1

  • Keywords has-patch has-unit-tests added

Image

@isabel_brison commented on PR #10906:


7 weeks ago
#2

There's an existing function wp_get_block_style_variation_name_from_class that accepts only a single param (class name). Would it be worth it to combine the logic into that existing functions, with the optional second param?

Yeah that's a good question. Also, it looks like wp_render_block_style_variation_support_styles which uses that helper function is rendering styles without checking whether they're registered or not. Should layout do that too?

Otherwise, I think we could consolidate the two functions into one.

Image

@andrewserong commented on PR #10906:


7 weeks ago
#3

Also, it looks like wp_render_block_style_variation_support_styles which uses that helper function is rendering styles without checking whether they're registered or not. Should layout do that too?

Oh, good point. Yeah, just as in wp_render_block_style_variation_support_styles, it looks like you're only using the $variation_name to access a key in the global styles data, so if there are any _unregistered_ classnames that are a match, it'd return null anyway. So yeah, given that function works that way, it sounds like you could use the same approach here as the style engine will sanitize output, anyway.

Still, I don't mind the additional check (and the idea of adding it into the existing function via a second param) as it does give an additional polish of making sure that we're only looking up values that _can_ exist.

Up to you, I think, if you prefer the clarity or "correctness" of the additional check, or the simplicity of re-using the existing function. (Personally I think it's a valid choice either way).

Image

@isabel_brison commented on PR #10906:


7 weeks ago
#4

Hmm I might try reusing the existing function for consistency.

Image

@isabel_brison commented on PR #10906:


7 weeks ago
#5

Hmm I might try reusing the existing function for consistency.

Actually, looking closer at wp_get_block_style_variation_name_from_class it might be a bit messy to reuse it because from $block['attrs']['className'] it returns an array of matching class names that include both the variation name and another name derived from the actual classname the styles are attached to, such as name--3. Might be best to keep the two separate functions after all. I'd leave the new one one in layout for now because it's the only place we're using it, and we can change that if we ever need to use it somewhere else.

I'll add a test for the new function too.

Image

@andrewserong commented on PR #10906:


7 weeks ago
#6

it might be a bit messy to reuse it because

Ah, fair enough! To make sure the name is distinct from the existing function would it be worth renaming to something like: wp_get_block_style_variation_name_from_registered_style? It'll be included in the global namespace, so it could help folks looking up functions if the names a little more different from each other.

I'd leave the new one one in layout for now because it's the only place we're using it, and we can change that if we ever need to use it somewhere else.

Up to you, but I'd lean toward co-locating it with the other function, just because they're both global functions, and it sometimes feels a little odd when similar utility functions are in different locations. Though I get that it also makes sense to keep it close to where it's mostly used! If so, you could always put layout somewhere in the function name if you wanted make it clear it's for use in the layout support.

Image

@isabel_brison commented on PR #10906:


7 weeks ago
#7

wp_get_block_style_variation_name_from_registered_style

Yeah that sounds good, I'll rename it!

Though I get that it also makes sense to keep it close to where it's mostly used! If so, you could always put layout somewhere in the function name if you wanted make it clear it's for use in the layout support.

It _could_ be used somewhere else, so it's probably best for the name not to imply otherwise. I like to have single use utilities live where they're used for code reading convenience, though that's a personal preference. But I don't think we have any clear cut rules around this sort of thing. If it ever gets used elsewhere in block supports, I'd be inclined to move it to src/wp-includes/block-supports/utils.php instead. But that feels like overkill for something used only in one place.

Image

@andrewserong commented on PR #10906:


7 weeks ago
#8

All sounds reasonable to me!

Image

@isabel_brison commented on PR #10906:


7 weeks ago
#9

Thanks for the reviews folks!

#10 Image @isabel_brison
7 weeks ago

  • Owner set to isabel_brison
  • Resolution set to fixed
  • Status changed from new to closed

In 61655:

Editor: fix grid layout for style variations defining blockGap.

Ensures the grid block columns computation takes into account any blockGap value output by an active block style variation.

Props isabel_brison, mukesh27, westonruter, andrewserong.
Fixes #64624.

Note: See TracTickets for help on using tickets.