This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
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.
Change default rows for product grid blocks to 3 #1613
Change default rows for product grid blocks to 3 #1613
Changes from 7 commits
53a8173
b14bb29
22a3bcb
a667b69
8b9861d
3ce8fa5
242c23d
ed4fad8
5455e31
caa33f2
9e87045
bb976ff
87cebea
9b1ae9d
9f9cb50
9bb9595
533a970
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thewc-blocks
handle) we can import it directly. This will also require updating thesideEffects
property inpackage.json
to keep webpack from dropping the import during treeshaking.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added to
assets/js/filters
and handled in sideEffects. You can see the latest code now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Added a check for the block prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ineditor/hocs
(staying connected with the filters there) maybe?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about in the filter js file itself since it's so specific?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍