Change default rows for product grid blocks to 3 #1613

mikejolley · 2020-01-20T16:27:53Z

Changes default rows for all grid blocks from 1 to 3. 3 feels like a better default, and specifically for the all products block it matches the 'grid' icon the block current has (3x3 grid of boxes).

Note: For SSR blocks, existing blocks rows will change to 3, unless the user already changed them away from 1. Gutenberg does not save default values to the block, so it's not possible to know if the user really set it to 1, or left the default enabled.

For the all products block we are handling deprecation, so existing blocks will not change. New blocks will be inserted with 3.

Additionally, all attributes, even defaults, will be saved to the block now. We're doing this using some filters. This will prevent future issues if defaults are changed.

Fixes #1262

How to test the changes in this Pull Request:

Add all products block, or any "products by x" block and confirm 3 rows and 3 columns by default. Ensure you're using a default WP theme in case of overrides.
Add a all products block using master (should have 1 row), save, then switch branches. Confirm there is still 1 row shown and there are no validation errors.
Save a all products block. Check the post content source. Ensure all attributes are saved to the HTML comment.

Changelog

Changed default rows and columns for product grid blocks to 3x3.

Aljullu

This seems to also update blocks that are currently in a page or post.

Steps to reproduce:

In master create a post with a Best Seeling Products block and notice it has only one row.
git checkout fix/1262-default-rows
Reload the post and see now it has three rows.

Should we do something to prevent that? I'm afraid changing the number of rows of already-added blocks might break some user pages.

mikejolley · 2020-01-20T16:50:47Z

@Aljullu Hmm I assumed the current setting would be saved to the existing blocks. Ill investigate.

mikejolley · 2020-01-20T17:14:14Z

@Aljullu @nerrad It seems that the only settings saved to blocks are those which differ from defaults, so anyone who left it as the default 1 value will be affected.

Struggling to see a solution to this. I guess we could introduce versioning i.e. an attribute thats stored to a block noting when it was saved/created and then changing default based on that, but it feels hacky.

Maybe just need to bite the bullet and swap defaults if we want to go this direction...

senadir · 2020-01-20T18:01:55Z

There is no direct way to deprecate default attributes, I agree with Mike, we should go ahead, while the adoption is small, chances are, not a lot of people have the 1/3 default, we should also leave a dev note explaining this, might be worth getting this into WC 3.9 and not wait until next version where the surface area will be larger

Aljullu · 2020-01-20T18:38:36Z

Would a solution like this work?

Update product grid blocks so they always save the columns and rows attributes from now on.
Make it so when adding a new block, it uses the new defaults (3x3).
For blocks that don't have the columns and rows attributes defined, keep using the old defaults (3x1).

Not sure if it's possible (I didn't take a deep look at the code) and it has the drawback that we will always need to maintain the old defaults. But thought it was worth sharing in case it seems feasible.

senadir · 2020-01-20T18:47:04Z

For blocks that don't have the columns and rows attributes defined, keep using the old defaults (3x1).

we don’t have a way to detect this, any newly inserted block or already saved block will fall under this case since default block attributes are parsed from the settings.attributes object of the block.

nerrad · 2020-01-20T18:50:13Z

I don't think we should merge this as is because it will lead to the broken behaviour for merchants that have published a block with the defaults because that's what they want.

Here's a suggested solution:

For server rendered blocks, we should keep the default attributes at 1 row. This will ensure that any existing blocks that don't have this attribute set will keep the original default.
We might be able to hook into block insertion and update the attributes for the block so that the row count is 3 (if the attribute is not set yet). That will help with only applying the defaults to NEW created blocks.

nerrad · 2020-01-20T20:15:42Z

Alright I played around with this and I still don't have a solution. However, I discovered the following:

If we just change the js side default to 3, and ensure we save attributes for server-side rendered blocks, this will ensure existing rendered blocks on the frontend will stay with 1 row, but as soon as that existing content is edited, the block would get updated to the new default and if the user saves without noticing, the frontend will be affected.
Doing the above (or what's in this pull) for the All Products block without a deprecation on the attribute will result in validation error because what is saved for the block is different from what the default is from the server. This isn't an issue if a merchant has picked a different value for the rows and saved.

I'm not sure on a resolution yet to change the default from 3 rows to 1 row for all grid type blocks but it will involve either:

deprecating and transforming the existing rows attribute for All Products to take care of potential block validation issues.
figuring out how to hook into the Gutenberg block insertion/validation to conditionally update what is used as the rows default when a block is first inserted (leaving things alone for existing blocks in the content). I spent some time seeing if I could figure this out but no luck yet.

Either way, this issue is looking like it's not as easy as I thought it might be initially :(

One final thing, I noticed that the default attributes php side are filled from theme support for the values if it exists. Does this mean that themes can override the defaults? If so, this could be problematic for the All Products block because it means switching themes that change the defaults could end up invalidating any existing All Products block that did not have it's defaults changed to something before saving.

mikejolley · 2020-01-21T10:32:17Z

Does this mean that themes can override the defaults?

Yes they can. And as we've seen, if defaults change, existing blocks change.

mikejolley · 2020-01-21T10:41:23Z

Looks like we're been stung by the decision in WordPress/gutenberg#7342 to not save defaults.

nerrad

This is a genius solution that tests really well. Excellent work on coming up with a usable solution Mike! I have a few comments (and I know you're probably already acting on some of them based on our discussion in slack).

assets/js/blocks/products/all-products/index.js

nerrad · 2020-01-21T16:08:40Z

assets/js/blocks/products/all-products/index.js

+addFilter(
+	'blocks.getBlockAttributes',
+	'woocommerce-blocks/get-block-attributes',
+	setBlockAttributeDefaults
+);
+
+addFilter(
+	'editor.BlockListBlock',
+	'woocommerce-blocks/with-default-attributes',
+	withDefaultAttributes
+);


As discussed in slack, let's move these into their own location. Since this is an editor only thing, we can probably put this in js/editor/filters.js then in the main entry file (which is built to the wc-blocks handle) we can import it directly. This will also require updating the sideEffects property in package.json to keep webpack from dropping the import during treeshaking.

I've added to assets/js/filters and handled in sideEffects. You can see the latest code now.

assets/js/blocks/products/utils.js

nerrad · 2020-01-21T16:16:30Z

assets/js/blocks/products/utils.js

+ * @return {Object} Filtered block attributes.
+ */
+export const setBlockAttributeDefaults = ( blockAttributes, blockType ) => {
+	Object.keys( blockType.attributes ).map( ( key ) => {


Would be good to only run this on our blocks. We're using a non-official property on the block config here, there's risk another block somewhere might be using defaults in a different way.

This also applies to the other filter implementations in this pull (I won't repeat this comment for withDefaultAttributes).

Done. Added a check for the block prefix.

nerrad · 2020-01-21T16:20:59Z

assets/js/hocs/with-default-attributes.js

+ *
+ *  @param	object BlockListBlock The BlockListBlock element.
+ */
+const withDefaultAttributes = createHigherOrderComponent(


I think this should be moved outside of the js/hocs folder. The hoc here has a very specific use case so it'd be nice to keep it separate for the more general hoc usages. probably would be good to keep it in editor/hocs (staying connected with the filters there) maybe?

What about in the filter js file itself since it's so specific?

nerrad

Looks great! However, I missed a prop mutation that was happening on withDefaultAttributes that will need addressed.

I tested the work with npm run build (which is when tree shaking happens, it doesn't happen on dev builds) and there was no breakage so sideEffects are configured correctly 👍 .

nerrad · 2020-01-21T20:38:59Z

assets/js/filters/block-list-block.js

+						typeof blockType.defaults !== 'undefined' &&
+						typeof blockType.defaults[ key ] !== 'undefined'
+					) {
+						props.attributes[ key ] = blockType.defaults[ key ];


I just realized, we shouldn't be directly mutating props. While on the surface this works well, I'm concerned about unexpected bugs surfacing in the future because of this mutation. Instead, I think we should create a new attributes object based off the old and feed that to an attributes prop. So essentially:

return function( { attributes, ...props } ) { //below would be inside your blockType.name conditional block ...basically creating a new object. attributes = Object.assign( {}, attributes ); // then you'd have your logic replacing undefined keys if needed. return <BlockListBlock attributes={ attributes } { ...props } /> }

…migration

nerrad

🎉 🌮

mikejolley added the status: needs review label Jan 20, 2020

mikejolley requested a review from a team January 20, 2020 16:27

mikejolley self-assigned this Jan 20, 2020

Aljullu reviewed Jan 20, 2020

View reviewed changes

mikejolley added 6 commits January 21, 2020 15:21

Change default rows to 3

53a8173

Handle all products rows change via deprecation

b14bb29

Prevent errors if object is undefined

22a3bcb

HOC to set default attribute values

a667b69

Separate default attributes from attribute definitions

8b9861d

Filters to set defaults

3ce8fa5

mikejolley force-pushed the fix/1262-default-rows branch from 5568b2a to 3ce8fa5 Compare January 21, 2020 15:21

Fix deprecation so attributes are not reordered

242c23d

nerrad reviewed Jan 21, 2020

View reviewed changes

mikejolley added 5 commits January 21, 2020 16:33

Move flters to index file and limit which blocks are affected

ed4fad8

Fix object assign

5455e31

Use typeof for undefined checks

caa33f2

Move hoc to filter file so it has context

9e87045

Fix up comment

bb976ff

nerrad suggested changes Jan 21, 2020

View reviewed changes

mikejolley added 2 commits January 22, 2020 12:25

Avoid mutating props

87cebea

Prevent multiple attributes by cloning props

9b1ae9d

mikejolley added 3 commits January 22, 2020 14:25

Force attributes to be saved to HTML in a specific order, and handle …

9f9cb50

…migration

useEffect to set attributes and avoid prop mutation

9bb9595

Switch to class component

533a970

nerrad approved these changes Jan 22, 2020

View reviewed changes

woocommercebot added status: ready to merge and removed status: needs review labels Jan 22, 2020

mikejolley merged commit bd8d9a1 into master Jan 22, 2020

mikejolley deleted the fix/1262-default-rows branch January 22, 2020 16:58

Aljullu mentioned this pull request Jan 23, 2020

Grid blocks that were already published grew from 1 to 3 rows #1637

Closed

mikejolley mentioned this pull request Jan 23, 2020

Move default population to class method instead of constructor #1638

Merged

nerrad added this to the 2.6.0 milestone Jan 27, 2020

Aljullu mentioned this pull request Mar 19, 2020

All blocks are broken in master and WP 5.0 #1914

Closed

nerrad mentioned this pull request May 25, 2020

Release: 2.6.0 #2554

Closed

17 tasks

Aljullu mentioned this pull request Jun 9, 2020

Update woocommerce/woocommerce-blocks to v2.7.0 woocommerce/woocommerce#26732

Merged

11 tasks

This was referenced Jun 25, 2020

Add storybook story for icon library #2787

Merged

Investigate default attributes not saving for blocks. #2789

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change default rows for product grid blocks to 3 #1613

Change default rows for product grid blocks to 3 #1613

mikejolley commented Jan 20, 2020 •

edited

Aljullu left a comment

mikejolley commented Jan 20, 2020

mikejolley commented Jan 20, 2020

senadir commented Jan 20, 2020

Aljullu commented Jan 20, 2020

senadir commented Jan 20, 2020

nerrad commented Jan 20, 2020

nerrad commented Jan 20, 2020

mikejolley commented Jan 21, 2020

mikejolley commented Jan 21, 2020

nerrad left a comment

nerrad Jan 21, 2020

mikejolley Jan 21, 2020

nerrad Jan 21, 2020

mikejolley Jan 21, 2020

nerrad Jan 21, 2020

mikejolley Jan 21, 2020

mikejolley Jan 21, 2020

nerrad left a comment

nerrad Jan 21, 2020

nerrad left a comment

Change default rows for product grid blocks to 3 #1613

Change default rows for product grid blocks to 3 #1613

Conversation

mikejolley commented Jan 20, 2020 • edited

How to test the changes in this Pull Request:

Changelog

Aljullu left a comment

Choose a reason for hiding this comment

mikejolley commented Jan 20, 2020

mikejolley commented Jan 20, 2020

senadir commented Jan 20, 2020

Aljullu commented Jan 20, 2020

senadir commented Jan 20, 2020

nerrad commented Jan 20, 2020

nerrad commented Jan 20, 2020

mikejolley commented Jan 21, 2020

mikejolley commented Jan 21, 2020

nerrad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerrad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerrad left a comment

Choose a reason for hiding this comment

mikejolley commented Jan 20, 2020 •

edited