Product Collection: Migrate displayLayout To supports.layout

[ObliviousHarmony]

Submission Review Guidelines:

• I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
• I have checked to ensure there aren't other open Pull Requests for the same update/change.
• I have reviewed my code for security best practices.
• Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

This pull request replaces the Product Collection block's custom displayLayout with Gutenberg's native layout block support. This allows us to take advantage of spacing support as well as the continued development of layout. This migration lets us avoid creating our own CSS styles and controls to handle editing margin, padding, and block spacing.

Closes #43307.

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

1. Create a Product Collection block and edit the layout on the collection block. Test it out!
2. There are new controls for Margin, Padding, and Block Spacing on the collection and template blocks to try out.
3. Create a Product Collection block on trunk:
   a. Confirm that the render callback works on this branch with an immigrated block.
   b. Confirm that the migration works and converts different configurations to the new attribute.

Changelog entry

• Automatically create a changelog entry from the details below.

Significance

• Patch
• Minor
• Major

Type

• Fix - Fixes an existing bug
• Add - Adds functionality
• Update - Update existing functionality
• Dev - Development related task
• Tweak - A minor adjustment to the codebase
• Performance - Address performance issues
• Enhancement - Improvement to existing functionality

Message

Comment

[github-actions]

Hi @imanish003, @kmanijak,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

[imanish003]

Hey @ObliviousHarmony,

Thank you for your fantastic work on this PR. You've done an amazing job, and I really appreciate your effort in going the extra mile and implementing migration for existing blocks 🚀

I've left several minor suggestions. Most of them are nitpicks, so please feel free to ignore any you disagree with 🙂

[imanish003]

Nit: When a new block is added to the post, the default COLUMN MODE is Manual. It would be better if we could change it to "Auto" because we want to keep the block responsive by default 🙂

[ObliviousHarmony]

I think that currently the caveats to "Auto" warrant using "Manual" by default. As we figure out what responsiveness looks like with this layout option we can re-asses. How does that sound?

[imanish003]

Just checking in case I missed it earlier, but could you please clarify the specific caveat you're referring to?

If I recall correctly, we've had this discussion before and decided to make the block responsive by default. With that in mind, I believe we should aim to maintain responsiveness if possible. IMO, sticking with manual might not offer the best user experience across different device sizes.

CC: @kmanijak

[ObliviousHarmony]

As it stands, the supports.layout option only supports a static columnCount or a dynamic minimumColumnWidth. There is currently no way to combine these two into a minimum width with a maximum number of columns in the way that our custom layout does. I did find a workaround (the custom CSS in the migration) but want to get confirmation from Gutenberg before moving forward with that and getting responsiveness working again.

If Gutenberg indicates that the solution won't be supported long-term then we will need to decide whether or not to move forward with this migration. Ultimately, if they don't agree, the only "responsiveness" supported by Gutenberg in this case would be the minimumColumnWidth which has no maximum column count.

Think of this PR as a deeply researched PoC 😄

[imanish003]

want to get confirmation from Gutenberg before moving forward with that and getting responsiveness working again.

🙌🏻

Ultimately, if they don't agree, the only "responsiveness" supported by Gutenberg in this case would be the minimumColumnWidth which has no maximum column count.

If they don't agree! I believe we will need to discuss this with @jarekmorawski before making a decision 🙂

Think of this PR as a deeply researched PoC 😄

Yeah! I can see that. Well done! 😀🚀

[ObliviousHarmony]

I believe we will need to discuss this with @jarekmorawski before making a decision

Yep! I'm just waiting to have some certainty around what that is or whether there is one to make!

[imanish003]

When switching between Auto and Manual modes, there is a noticeable flickering in the UI, as captured video below. Is it possible to fix it? 🤔

Screen.Recording.2024-04-05.at.2.22.27.PM.mov

[ObliviousHarmony]

To be honest, I'm not sure what causes this! @kmanijak had an idea but that didn't seem to make a difference. For what it's worth, regarding Karol's comment, I don't think this ever caused unnecessary re-renders in the first place.

Do you have any ideas about what might be happening here? We're changing the control based on isAutoColumnLayout so it should be atomic but it isn't?

[imanish003]

I think we can fix it while addressing #46255 (comment) below. For example, refactoring the code as follows could potentially resolve the flickering issue:

<ToolsPanelItem
	label={ minimumColumnWidthLabel }
	hasValue={ () => true }
	isShownByDefault
>
	{ isAutoColumnLayout ? (
		<UnitControl
			label={ minimumColumnWidthLabel }
			value={ templateLayout.minimumColumnWidth as string }
			onChange={ onMinimumColumnWidthChange }
		/>
	) : (
		<RangeControl
			label={ columnCountLabel }
			onChange={ onColumnCountChange }
			value={ templateLayout.columnCount as number }
			min={ 2 }
			max={ 6 }
		/>
	) }
</ToolsPanelItem>

I haven't delved deeply into it, but my initial thought is that the flickering may result from the conditional rendering of the ToolsPanelItem component, particularly because it also needs to update the dropdown menu (see screenshot) whenever we render ToolsPanelItem. However, this is just an initial hypothesis; further investigation is needed to confirm.

[kmanijak]

Amazing work!

1. We move to Gutenberg native solution
2. As far I can see you were able to preserve all of the requirements in terms of which options are available where, right?

I played around with this and left some comments, some of them should be addressed before merging but overall, great work. 😆

There's one more thing you couldn't know about (especially because it's not covered with tests). There's migration from Products (Beta) to Product Collection block:

There's:

• migration from Products to Product Collection (mapping layout here: https://github.com/woocommerce/woocommerce/blob/trunk/plugins/woocommerce-blocks/assets/js/blocks/migration-products-to-product-collection/migration-from-products-to-product-collection.ts#L121)
• possibility to revert, so from Product Collection to Products (mapping layout here: https://github.com/woocommerce/woocommerce/blob/trunk/plugins/woocommerce-blocks/assets/js/blocks/migration-products-to-product-collection/migration-from-product-collection-to-products.ts#L91)

These mappings should be adjusted but at the same time, I think we may think about removing the option to migrate. I mean... options is available for months and as both blocks diverge more and more from each other I don't like us maintaining migration for too long. So I'll raise the discussion if and how long we want to preserve that option.

[kmanijak]

Suggested change

	const { displayLayout = null, ...newAttributes } = attributes;
	const { displayLayout, ...newAttributes } = attributes;

Nitpick: Default value will be assigned only if displayLayout is undefined and that will be caught in line 23 anyway so I think it's simpler without defaulting to null.

[kmanijak]

It's a nice approximation! But the previous solution was based on specific minimum product width 150px (here) so I'd use DEFAULT_LAYOUT_GRID_MINIMUM_COLUMN_WIDTH here with no calculations.

columns were used as "max number of columns" so that's not a good source for the calculations as it results in wrong results for smaller screens.

When I tested the PR I encountered a case in which Product Collection was 3-column on trunk and 4-column on this branch. Setting the minimumColumnWidth to 150px in Inspector Controls allowed me to get the same result as on trunk.

Or am I missing something 😅

[kmanijak]

Second thoughts. We're not able to migrate one logic to another, because:

• old logic was based on maximum number of columns and minimum column width
• new logic is based on minimum column width only (no upper limit).

So... Hmm... There's no right solution here 🙈 And I'm no longer convinced to my suggestion as well.

[ObliviousHarmony]

I went with 90% because it felt like a good approximation. At your prompting, however, I took a deeper look. We dealt with this in the old styles using this CSS:

$gap-count: calc(#{ $i } - 1);
$total-gap-width: calc(#{ $gap-count } * #{ $grid-gap });
$max-product-width: calc((100% - #{ $total-gap-width }) / #{ $i });

&.columns-#{ $i } {
  grid-template-columns: repeat(auto-fill, minmax(max(#{ $min-product-width }, #{ $max-product-width }), 1fr));
}

As we can see, it includes a calculation for the gap width and uses a percentage.

A caveat, however, is that we're now using a dynamic gap value through supports.spacing.blockGap. Even if we used this CSS class we wouldn't be able to replicate the previous responsive behavior!

Using the above calculation and the default blockGap value I will take another pass at having the percentage be identical.

[ObliviousHarmony]

Thanks again @kmanijak! I went back and adjusted the math. It should now work in every case, please test it out!

Assuming it works, I'll make some changes to the editor controls so that they look correct. As an aside, if we support block spacing, we can't support the kind of responsive layout we had before without writing our own block spacing and abandoning this migration altogether.

This was fine before because the block gap was hardcoded. With it being configurable like this, our CSS classes can't know the size and therefore can't do the necessary math.

[kmanijak]

+1 on this!