Convert filter blocks to inner blocks #6978
Conversation
|
The release ZIP for this PR is accessible via: |
Script Dependencies ReportThe
This comment was automatically generated by the |
|
Size Change: +42.2 kB (+5%) 🔍 Total Size: 958 kB
ℹ️ View Unchanged
|
This reverts commit 8b6cb7c.
|
@tjcafferkey @albarin I managed to render the filter wrapper and its inner blocks on the front end. Here is how it works on 49236d8. Active Filters and Price Filters are supported for now only:
Screen.Recording.2022-09-01.at.11.02.35.movThere is a small blocker now that I'd love to have your feedback on. It's the block naming. In the following picture, I use two different approaches for naming wrapper and inner filter blocks:
Which one do you think is better? I lean toward the second one as it introduces fewer changes to the end users.
|
@dinhtungdu I completely agree with you, prefer the second option, having |
dinhtungdu
changed the title
I updated the wrapper block to remove @albarin @tjcafferkey In 3ba7237 (refactored in c3b9c67), I try a different way to upgrade the block. As the existing filter blocks will continue working without users' action, I think a toolbar button to upgrade the blocks is a better way to convert the old blocks into the new wrapper blocks. Here is how it looks for the Active Filters block (I only updated that block FYI):
|
dinhtungdu
changed the title
|
@tjcafferkey @gigitux Marking this PR ready for review as all filter blocks e2e tests have been updated. The failed tests are related to checkout and mini cart which is being addressed in #7206. For the remaining regression in #6978 (comment):
|
|
I have done some user testing on this including with the classic Storefront theme as widgets and it is looking good. I am yet to review the code though as there are a lot of changes, but just to answer some of your comments above:
Even though it's not idea, I don't see this as a blocker, or a regression from a users point of view. There is no reason we cannot follow this up as the value this provides to merchants and users is greater than the maintenance overhead we might inherit in my opinion.
Nice work!
Again I do not see this as a blocker. Great work so far @dinhtungdu, I will continue reviewing the code likely tomorrow. |
| "color": { | ||
| "text": true, | ||
| "background": false | ||
| } | ||
| }, | ||
| "lock": false |
There was a problem hiding this comment.
Just to confirm my assumption: We set the lock status to false initially for users who already have the block inserted, but once they upgrade to the new inner blocks version we set this to true so users cannot remove the inner blocks?
There was a problem hiding this comment.
We're disabling the lock support which only disables the ability to toggle the lock UI of the block, not the block locking itself.
When users upgrade or add the new filter block, we lock the remove of control blocks by setting the lock attribute object.
We can skip disabling lock support here, but then users can toggle the filter control blocks locking using the lock UI.
There was a problem hiding this comment.
With lock support disabled: ("lock": false)
With lock support enabled: (default)
| template={ [ | ||
| [ | ||
| 'core/heading', | ||
| { level: 3, content: attributes.heading || '' }, |
There was a problem hiding this comment.
level: 3 does this mean that even if the user has the title level heading set to Level 2 before upgrading that after the upgrade this will be changed to 3? If so, is there any reason we settled on level 3 heading?
There was a problem hiding this comment.
Right, we should use the current heading level if available.
There was a problem hiding this comment.
Right, we should use the current heading level if available.
We're actually doing this in assets/js/blocks/filter-wrapper/upgrade.tsx.
level: 3does this mean that even if the user has the title level heading set to Level 2 before upgrading that after the upgrade this will be changed to 3? If so, is there any reason we settled on level 3 heading?
We specify level: 3 in the assets/js/blocks/filter-wrapper/edit.tsx as the default level for the heading block. 3 is also the current default heading level of (legacy) filter blocks.
patterns/filters.php
Outdated
| <!-- wp:woocommerce/filter-wrapper --> | ||
| <div class="wp-block-woocommerce-filter-wrapper"> | ||
| <!-- wp:heading {"level":3} --> | ||
| <h3>Active Filters</h3> |
There was a problem hiding this comment.
Not a blocker, but I wonder if internationalization is handled with these values?
There was a problem hiding this comment.
Because it's a PHP file, we can use WP i18n functions here.
|
The code LGTM, I've asked some questions which are non-blockers but before approving can we check out this failing test please?
Great work @dinhtungdu ! |
That test isn't really failed but passed after 1 failed attempt. I'd classify it as a flaky test instead. I'm updating the PR to preserve the heading level after upgrading, let's see how the tests run then. |
|
@tjcafferkey Thanks for the review! fixed the i18n issue for the patterns and replied to your other comments, please take another look at this! Thank you! |
|
@dinhtungdu I cannot approve my own PR but this PR now LGTM! 😄 It tests well, and the code LGTM. Im not sure whether you would like to wait for @gigitux to also look over this seeing as we both worked on this PR so he may spot something we have not. |
|
@dinhtungdu @tjcafferkey Hey, I'm having a look, one thing I found is that if I insert a |
|
Merging this, failing tests are unrelated to this PR as they are Mini Cart related. |
* register filter wrapper block * register block variation * rename the active filters wrapper * prevent 404 error * Revert "prevent 404 error" This reverts commit 8b6cb7c. * render parent wrapper block * support price filter block * hide the active filter block from inserter * swap the title of wrapper and inner filter block for active filters * hide the legacy heading for the price filter * update block title and description for active filters and price filter * remove heading control for price filter * remove heading control for active filters * update pattern * try: upgrade button * limit the number of inner block to 2 * prevent removing the inner filter block * Revert "prevent removing the inner filter block" This reverts commit 83b7fee. * convert stock filter to inner block * refactor block upgrade button to share between filter blocks * update default heading * update pattern * update icon and title * Fix stock filter error by importing translations package * Upgrade Active Filters name to Active Filter Controls * Add upgrade support to price filter * Convert attribute filter to inner block (woocommerce#7101) * wip: convert attribute filter to inner block * fix: render inner attribute filter block on the front end * refactor: inner block wrapper, extract the attribute parsing logic into a utility Co-authored-by: Tom Cafferkey <[email protected]> * Set correct attribute on the new filter blocks when they are upgraded * Use the Warning component to display the upgrade message so it is consistent with Gutenberg * address code review * better detect legacy block to show the upgrade notice * rename UpgradeToolbarButton to UpgradeNotice * add upgrade notice to the stock filter block * rename InnerBlockWrapper to BlockWrapper * attribute-filter: control wrapper visibility * passing block attributes down to inner active filters control block * fix styling of inner attribute filter control block * passing attribute to inner price filter control block * passing down the attribute to inner stock filter control block * remove unneccessary parsing * use default scope for variations * fix default attribute values * use default block appender * fix: lock control blocks * remove dynamic title code from attribute filter block * register active filters as variation and set it to the default that overrides the base block * fix isActive for default variation * fix: isActive logic for the active filters block * register side effect * fix ts error * e2e: fix active filters block backend test * e2e: fix frontend active filters test * e2e: fix attribute filter test * e2e: fix price filter test * e2e: fix stock filter test * e2e: update fixture * e2e: fix attribute filter test * remove invalid test * e2e: update heading selector for price filter in backend test * e2e: fixe backend price filter heading test * fix: patterns i18n * fix: heading level when upgrading the block Co-authored-by: Tung Du <[email protected]>
Fixes #6845
Also closes #6543
In this PR, we convert filter blocks into parent blocks containing a
core/headingblock and the actual filter block. This makes it easier and clearer for users to style the filter blocks:woocommerce/filter-wrapperblock is a parent block that contains thecore/headingblock and the actual filter block.woocommerce/filter-wrapper.refthat is passed down to the filter block using context. The visibility of the wrapper block is controlled inside the actual filter block using thatref.renderInnerBlocks.2for the heading and inner filter block.Screenshots
Screen.Recording.2022-09-01.at.11.02.35.mov
Testing
Automated Tests
User Facing Testing
Insert new blocks:
Upgrade existing blocks
trunk.WooCommerce Visibility
Changelog