Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Product Collection: Migrate displayLayout To supports.layout #46255

Draft
wants to merge 9 commits into
base: trunk
Choose a base branch
from
+463 −210
Draft

Product Collection: Migrate displayLayout To supports.layout #46255

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2d59a39
Replaced `displayLayout` With `layout`
ObliviousHarmony Apr 3, 2024
e842e92
Added "Minimum Column Width" Option
ObliviousHarmony Apr 4, 2024
e847a6d
Adding Collection Layout + Spacing Options
ObliviousHarmony Apr 5, 2024
97ea00a
Added `displayLayout` Deprecation
ObliviousHarmony Apr 5, 2024
2e4ad27
Made Unit Draggable
ObliviousHarmony Apr 5, 2024
cc126cf
Changelog
ObliviousHarmony Apr 5, 2024
991b5f9
Style Fixes
ObliviousHarmony Apr 5, 2024
e9b45f1
Fixed Re-Renders Causing UI Glitches
ObliviousHarmony Apr 5, 2024
2d0a30f
Revised displayLayout Responsive Migration
ObliviousHarmony Apr 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 deletions plugins/woocommerce-blocks/assets/js/blocks/product-collection/block.json
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! 😀🚀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
image

Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"tagName": {
"type": "string"
},
"displayLayout": {
"templateLayout": {
ObliviousHarmony marked this conversation as resolved.
Show resolved Hide resolved
"type": "object"
},
"convertedFromProducts": {
Expand All @@ -44,6 +44,7 @@
"queryId": "queryId",
"query": "query",
"displayLayout": "displayLayout",
"templateLayout": "templateLayout",
ObliviousHarmony marked this conversation as resolved.
Show resolved Hide resolved
"queryContextIncludes": "queryContextIncludes",
"collection": "collection"
},
Expand All @@ -52,7 +53,12 @@
"align": [ "wide", "full" ],
"anchor": true,
"html": false,
"__experimentalLayout": true,
"layout": true,
"spacing": {
"margin": true,
"padding": true,
"blockGap": true
},
"interactivity": true
}
}
21 changes: 13 additions & 8 deletions plugins/woocommerce-blocks/assets/js/blocks/product-collection/collections/best-sellers.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,13 @@ import {
DEFAULT_ATTRIBUTES,
INNER_BLOCKS_PRODUCT_TEMPLATE,
} from '../constants';
import { CoreCollectionNames, CoreFilterNames } from '../types';
import {
CoreCollectionNames,
CoreFilterNames,
LayoutOptions,
ProductCollectionAttributes,
ProductCollectionQuery,
} from '../types';

const collection = {
name: CoreCollectionNames.BEST_SELLERS,
Expand All @@ -23,15 +29,14 @@ const collection = {
scope: [],
};

const attributes = {
...DEFAULT_ATTRIBUTES,
displayLayout: {
type: 'flex',
columns: 5,
shrinkColumns: true,
const attributes: ProductCollectionAttributes = {
...( DEFAULT_ATTRIBUTES as ProductCollectionAttributes ),
templateLayout: {
type: LayoutOptions.GRID,
columnCount: 5,
},
query: {
...DEFAULT_ATTRIBUTES.query,
...( DEFAULT_ATTRIBUTES.query as ProductCollectionQuery ),
inherit: false,
orderBy: 'popularity',
order: 'desc',
Expand Down
21 changes: 13 additions & 8 deletions plugins/woocommerce-blocks/assets/js/blocks/product-collection/collections/featured.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,13 @@ import {
DEFAULT_ATTRIBUTES,
INNER_BLOCKS_PRODUCT_TEMPLATE,
} from '../constants';
import { CoreCollectionNames, CoreFilterNames } from '../types';
import {
CoreCollectionNames,
CoreFilterNames,
LayoutOptions,
ProductCollectionAttributes,
ProductCollectionQuery,
} from '../types';

const collection = {
name: CoreCollectionNames.FEATURED,
Expand All @@ -23,15 +29,14 @@ const collection = {
scope: [],
};

const attributes = {
...DEFAULT_ATTRIBUTES,
displayLayout: {
type: 'flex',
columns: 5,
shrinkColumns: true,
const attributes: ProductCollectionAttributes = {
...( DEFAULT_ATTRIBUTES as ProductCollectionAttributes ),
templateLayout: {
type: LayoutOptions.GRID,
columnCount: 5,
},
query: {
...DEFAULT_ATTRIBUTES.query,
...( DEFAULT_ATTRIBUTES.query as ProductCollectionQuery ),
inherit: false,
featured: true,
perPage: 5,
Expand Down
16 changes: 9 additions & 7 deletions plugins/woocommerce-blocks/assets/js/blocks/product-collection/collections/new-arrivals.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ import {
CoreCollectionNames,
CoreFilterNames,
ETimeFrameOperator,
LayoutOptions,
ProductCollectionAttributes,
ProductCollectionQuery,
} from '../types';

const collection = {
Expand All @@ -27,15 +30,14 @@ const collection = {
scope: [],
};

const attributes = {
...DEFAULT_ATTRIBUTES,
displayLayout: {
type: 'flex',
columns: 5,
shrinkColumns: true,
const attributes: ProductCollectionAttributes = {
...( DEFAULT_ATTRIBUTES as ProductCollectionAttributes ),
templateLayout: {
type: LayoutOptions.GRID,
columnCount: 5,
},
query: {
...DEFAULT_ATTRIBUTES.query,
...( DEFAULT_ATTRIBUTES.query as ProductCollectionQuery ),
inherit: false,
orderBy: 'date',
order: 'desc',
Expand Down
21 changes: 13 additions & 8 deletions plugins/woocommerce-blocks/assets/js/blocks/product-collection/collections/on-sale.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,13 @@ import {
DEFAULT_ATTRIBUTES,
INNER_BLOCKS_PRODUCT_TEMPLATE,
} from '../constants';
import { CoreCollectionNames, CoreFilterNames } from '../types';
import {
CoreCollectionNames,
CoreFilterNames,
LayoutOptions,
ProductCollectionAttributes,
ProductCollectionQuery,
} from '../types';

const collection = {
name: CoreCollectionNames.ON_SALE,
Expand All @@ -26,15 +32,14 @@ const collection = {
scope: [],
};

const attributes = {
...DEFAULT_ATTRIBUTES,
displayLayout: {
type: 'flex',
columns: 5,
shrinkColumns: true,
const attributes: ProductCollectionAttributes = {
...( DEFAULT_ATTRIBUTES as ProductCollectionAttributes ),
templateLayout: {
type: LayoutOptions.GRID,
columnCount: 5,
},
query: {
...DEFAULT_ATTRIBUTES.query,
...( DEFAULT_ATTRIBUTES.query as ProductCollectionQuery ),
inherit: false,
woocommerceOnSale: true,
perPage: 5,
Expand Down
9 changes: 3 additions & 6 deletions ...woocommerce-blocks/assets/js/blocks/product-collection/collections/product-collection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { Icon, loop } from '@wordpress/icons';
* Internal dependencies
*/
import { DEFAULT_ATTRIBUTES, INNER_BLOCKS_TEMPLATE } from '../constants';
import { CoreCollectionNames } from '../types';
import { CoreCollectionNames, ProductCollectionAttributes } from '../types';

const collection = {
name: CoreCollectionNames.PRODUCT_CATALOG,
Expand All @@ -21,11 +21,8 @@ const collection = {
scope: [],
};

const attributes = {
...DEFAULT_ATTRIBUTES,
query: {
...DEFAULT_ATTRIBUTES.query,
},
const attributes: ProductCollectionAttributes = {
...( DEFAULT_ATTRIBUTES as ProductCollectionAttributes ),
ObliviousHarmony marked this conversation as resolved.
Show resolved Hide resolved
collection: collection.name,
hideControls: [],
};
Expand Down
21 changes: 13 additions & 8 deletions plugins/woocommerce-blocks/assets/js/blocks/product-collection/collections/top-rated.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,13 @@ import {
DEFAULT_ATTRIBUTES,
INNER_BLOCKS_PRODUCT_TEMPLATE,
} from '../constants';
import { CoreCollectionNames, CoreFilterNames } from '../types';
import {
CoreCollectionNames,
CoreFilterNames,
LayoutOptions,
ProductCollectionAttributes,
ProductCollectionQuery,
} from '../types';

const collection = {
name: CoreCollectionNames.TOP_RATED,
Expand All @@ -26,15 +32,14 @@ const collection = {
scope: [],
};

const attributes = {
...DEFAULT_ATTRIBUTES,
displayLayout: {
type: 'flex',
columns: 5,
shrinkColumns: true,
const attributes: ProductCollectionAttributes = {
...( DEFAULT_ATTRIBUTES as ProductCollectionAttributes ),
templateLayout: {
type: LayoutOptions.GRID,
columnCount: 5,
},
query: {
...DEFAULT_ATTRIBUTES.query,
...( DEFAULT_ATTRIBUTES.query as ProductCollectionQuery ),
inherit: false,
orderBy: 'rating',
order: 'desc',
Expand Down
17 changes: 10 additions & 7 deletions plugins/woocommerce-blocks/assets/js/blocks/product-collection/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@ import {
TProductCollectionOrder,
TProductCollectionOrderBy,
ProductCollectionQuery,
ProductCollectionDisplayLayout,
ProductCollectionLayout,
ObliviousHarmony marked this conversation as resolved.
Show resolved Hide resolved
LayoutOptions,
ProductCollectionLayoutGrid,
} from './types';
import { ImageSizing } from '../../atomic/blocks/product-elements/image/types';
import { VARIATION_NAME as PRODUCT_TITLE_ID } from './variations/elements/product-title';
Expand Down Expand Up @@ -63,13 +64,15 @@ export const DEFAULT_QUERY: ProductCollectionQuery = {
priceRange: undefined,
};

export const DEFAULT_LAYOUT_GRID_COLUMNS = 3;
export const DEFAULT_LAYOUT_GRID_MINIMUM_COLUMN_WIDTH = '150px';
ObliviousHarmony marked this conversation as resolved.
Show resolved Hide resolved

export const DEFAULT_ATTRIBUTES: Partial< ProductCollectionAttributes > = {
query: DEFAULT_QUERY,
tagName: 'div',
displayLayout: {
templateLayout: {
type: LayoutOptions.GRID,
columns: 3,
shrinkColumns: true,
columnCount: DEFAULT_LAYOUT_GRID_COLUMNS,
},
queryContextIncludes: [ 'collection' ],
forcePageReload: false,
Expand All @@ -84,13 +87,13 @@ export const getDefaultQuery = (
inherit: getDefaultValueOfInheritQueryFromTemplate(),
} );

export const getDefaultDisplayLayout = () =>
DEFAULT_ATTRIBUTES.displayLayout as ProductCollectionDisplayLayout;
export const getDefaultTemplateLayout = () =>
DEFAULT_ATTRIBUTES.templateLayout as ProductCollectionLayoutGrid;

export const getDefaultSettings = (
currentAttributes: ProductCollectionAttributes
): Partial< ProductCollectionAttributes > => ( {
displayLayout: getDefaultDisplayLayout(),
templateLayout: getDefaultTemplateLayout(),
query: getDefaultQuery( currentAttributes.query ),
} );

Expand Down