Migrate Cart i1 to Cart i2 #4973
Conversation
senadir
force-pushed
the
add/migrate-cart-i1-to-i2
branch
from
9baed2f to
ea42468
Compare
|
Size Change: -139 kB (-11%) 👏 Total Size: 1.1 MB
ℹ️ View Unchanged
|
| const EmptyCart = ( { content } ) => { | ||
| useEffect( () => { | ||
| dispatchEvent( 'wc-blocks_render_blocks_frontend', { | ||
| element: document.body.querySelector( | ||
| '.wp-block-woocommerce-cart' | ||
| ), | ||
| } ); | ||
| }, [] ); | ||
| return <RawHTML>{ content }</RawHTML>; | ||
| }; |
There was a problem hiding this comment.
This was moved to Empty cart block, no RawHTML needed
| @@ -75,19 +86,32 @@ const Block = ( { emptyCart, attributes, scrollToTop } ) => { | |||
| }; | |||
| }, [ scrollToTop ] ); | |||
|
|
|||
There was a problem hiding this comment.
I used the updated ScrollonError from Checkout.
| @@ -1,47 +1,54 @@ | |||
| /* tslint:disable */ | |||
There was a problem hiding this comment.
We will handle TS in a follow up, this is useful to keep a good blame history.
| const CartFrontend = ( props ) => { | ||
| return ( | ||
| <StoreSnackbarNoticesProvider context="wc/cart"> | ||
| <StoreNoticesProvider context="wc/cart"> | ||
| <SlotFillProvider> | ||
| <CartProvider> | ||
| <Block { ...props } /> | ||
| </CartProvider> | ||
| </SlotFillProvider> | ||
| </StoreNoticesProvider> | ||
| </StoreSnackbarNoticesProvider> | ||
| ); | ||
| }; |
| migrate: ( attributes, innerBlocks ) => { | ||
| return [ | ||
| attributes, | ||
| [ | ||
| createBlock( 'woocommerce/filled-cart-block' ), | ||
| createBlock( | ||
| 'woocommerce/empty-cart-block', | ||
| {}, | ||
| innerBlocks | ||
| ), | ||
| ], | ||
| ]; | ||
| }, |
There was a problem hiding this comment.
This function takes the previous inner Blocks from Cart i1 and place it in the correct place.
| @@ -80,7 +80,32 @@ protected function render( $attributes, $content ) { | |||
| wp_dequeue_script( 'selectWoo' ); | |||
| wp_dequeue_style( 'select2' ); | |||
|
|
|||
| return $content . $this->get_skeleton(); | |||
| // If the content contains new inner blocks, it means we're in the newer version of Cart. | |||
| $regex_for_new_block = '/<div[\n\r\s\ta-zA-Z0-9_\-=\'"]*data-block-name="woocommerce\/filled-cart-block"[\n\r\s\ta-zA-Z0-9_\-=\'"]*>/mi'; | |||
There was a problem hiding this comment.
We check if we're running Cart i2.
|
|
||
| $is_new = preg_match( $regex_for_new_block, $content ); | ||
|
|
||
| if ( ! $is_new ) { |
There was a problem hiding this comment.
If we're not, then we're going to insert the default template
|
|
||
| if ( ! $is_new ) { | ||
| // This fallback needs to match the default templates defined in our Blocks. | ||
| $inner_blocks_html = '$0 |
There was a problem hiding this comment.
$0 is used here to replace the old block opening, it supports custom calsses.
| <div data-block-name="woocommerce/empty-cart-block" class="wp-block-woocommerce-empty-cart-block"> | ||
| '; | ||
|
|
||
| $content = preg_replace( '/<div class="[a-zA-Z0-9_\- ]*wp-block-woocommerce-cart[a-zA-Z0-9_\- ]*">/mi', $inner_blocks_html, $content ); |
There was a problem hiding this comment.
Insert template at start.
src/BlockTypes/Cart.php
Outdated
| '; | ||
|
|
||
| $content = preg_replace( '/<div class="[a-zA-Z0-9_\- ]*wp-block-woocommerce-cart[a-zA-Z0-9_\- ]*">/mi', $inner_blocks_html, $content ); | ||
| $content = substr_replace( $content, '</div></div>', -6 ); |
There was a problem hiding this comment.
insert a closing div at the end.
There was a problem hiding this comment.
What exactly is the function of this line? I understand it replaces the last 6 chars with </div></div> am I correct?
Why not just add </div></div> to the end of the $inner_blocks_html string?
assets/js/blocks/cart-checkout/cart/full-cart/cart-line-items-table.tsx
Outdated
Show resolved
Hide resolved
mikejolley
added
status: needs review
block: cart
|
Is there a reason we see Accepted Payment Methods grayed out when trying to add a new block on the right column of the cart? |
| emptyCart={ null } | ||
| attributes={ { | ||
| isShippingCalculatorEnabled: false, | ||
| } } |
There was a problem hiding this comment.
We no longer need to define emptyCart or attributes here.
| // ["`select` control in `@wordpress/data-controls` is deprecated. Please use built-in `resolveSelect` control in `@wordpress/data` instead."] | ||
| expect( console ).toHaveWarned(); |
| await waitFor( () => expect( fetchMock ).toHaveBeenCalled() ); | ||
| expect( container ).toMatchSnapshot(); | ||
| expect( screen.getByText( /Tax/i ) ).toBeInTheDocument(); |
There was a problem hiding this comment.
Removed snapshots in favor of RTL selectors.
| <Cart attributes={ attributes }> | ||
| <FilledCart> | ||
| <ItemsBlock> | ||
| <LineItemsBlock /> | ||
| </ItemsBlock> | ||
| <TotalsBlock> | ||
| <OrderSummaryBlock | ||
| showRateAfterTaxName={ showRateAfterTaxName } | ||
| isShippingCalculatorEnabled={ | ||
| isShippingCalculatorEnabled | ||
| } | ||
| /> | ||
| <ExpressPaymentBlock /> | ||
| <ProceedToCheckoutBlock /> | ||
| <AcceptedPaymentMethodsIcons /> | ||
| </TotalsBlock> | ||
| </FilledCart> | ||
| <EmptyCart> | ||
| <p>Empty Cart</p> | ||
| </EmptyCart> | ||
| </Cart> |
There was a problem hiding this comment.
Since we're running an innerBlocks structure now, we need to construct the block ourselves.
Yes, that block is optional and so can be inserted and removed, it's also restricted to a single use, so it's grayed out because it's already inserted. |
|
I think those can be updated automatically the next time we run the automation script? |
There was a problem hiding this comment.
@senadir I'm gonna post this review so that you can look into my comments and I'll post individual comments for the rest of the review
assets/js/blocks/cart-checkout/cart/inner-blocks/cart-order-summary-block/block.tsx
Show resolved
Hide resolved
assets/js/blocks/cart-checkout/cart/inner-blocks/cart-order-summary-block/attributes.tsx
Show resolved
Hide resolved
assets/js/blocks/cart-checkout/cart/inner-blocks/empty-cart-block/edit.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
I've finished my review, things I have tested:
- Coupon code
- Transfer of cart attributes from i1 to i2
- Filters in cart
- Interaction with the editor
- Interaction with the cart on the frontend
- Express payment
- 0 total
- Shipping calculator
- Inserting custom blocks
Things I've found no to work:
- setting custom classes for inner blocks, guess it will be fixed in Cart i2: additional classes for inner blocks do not appear on the front-end #4962
- disabling the Shipping calculator/ passing this setting from Cart i1 to Cart i2
- "shows empty cart when changing the view" test was failing randomly for me, I've pushed a change to see if it fixes it
yes, running |
I managed to get this to work on the editor, no luck on the frontend. It would make the block too complicated for little gain, we're going to issue a dev note with the block stating that people need to resave if they changed attributes. The attributes are mostly cosmetic and provide good defaults already, so it's not a big issue. |
as per request from @ralucaStan, I'm expanding on this section. To get old attributes support in the new layout, we need to pass them via This essentially introduces extra code for a simple migration code that we later need to delete. I didn't find value in doing this, especially as what we're trying to do is carry over the value of two cosmetic attributes. Having the wrong value in the frontend has little impact (e.g: showing the taxes % or not, showing shipping calculator or not). The attributes value isn't lost, it's saved in the editor block migration code (which is built in for that) and would reapply if the merchants saves the editor page again. |
There was a problem hiding this comment.
Apart from the issues already pointed out by @ralucaStan already about attribute migration and custom classes, there is also the issue of the width of the block, the content doesn't stretch like it did in the old cart. I don't think this is too much of a big deal but just flagging it anyway.
edit: the width setting works fine on the front-end
| Old cart | New cart (after migration, before saving) |
|---|---|
 |
 |
Happy to approve on the basis that:
- the migration from old cart to new cart works well and doesn't break the site, and the planned dev note will address what we've pointed out.
- The comment I left about translation is addressed/defended.
- The comment I left about the template is explained.
| [ | ||
| { | ||
| view: 'woocommerce/filled-cart-block', | ||
| label: __( 'Filled Cart', 'woo-gutenberg-products-block' ), |
There was a problem hiding this comment.
Noting that this previously read Full Cart
There was a problem hiding this comment.
Yes, we changed from Full to Filled because Full didn't make sense, while filled assumes there's at least one item.
src/BlockTypes/Cart.php
Outdated
| '; | ||
|
|
||
| $content = preg_replace( '/<div class="[a-zA-Z0-9_\- ]*wp-block-woocommerce-cart[a-zA-Z0-9_\- ]*">/mi', $inner_blocks_html, $content ); | ||
| $content = substr_replace( $content, '</div></div>', -6 ); |
There was a problem hiding this comment.
What exactly is the function of this line? I understand it replaces the last 6 chars with </div></div> am I correct?
Why not just add </div></div> to the end of the $inner_blocks_html string?
github-actions
bot
added
status: ready to merge
and removed
status: needs review
labels
This reverts commit 79d55de.
Automatic update after running npm run build:docs
senadir
force-pushed
the
add/migrate-cart-i1-to-i2
branch
from
15cb983 to
630b6be
Compare
frontdevde
added
the
type: enhancement
* move empty cart * remove Cart and rename Cart i2 to Cart * graduate blocks * setup template migration from Cart i1 to Cart i2 * back to js so we have a good diff * add migration * fix bug in empty cart template * add useForceLayout hook to edit * migrate from old block to new block * migrate styles * respect align * add tests * Include latest cart line item improvements from cart-i1 * Missing changes from cart-i1 * Line items table should be disabled * Fix e2e tests for cart i2 * update tests to adapt for inner blocks * update select to resolveSelect to remove warning checker * rename test/block to test/index * move blocks to their own file * undo rename to keep diff clean * remove .tsx and update jest config * Revert "update select to resolveSelect to remove warning checker" This reverts commit 79d55de. * revert resolveControl * Fix empty cart editor E2E test by scrolling to the view switch * parse attributes for order summary block * migrate attributes when resaving * Update documentation Automatic update after running npm run build:docs * add align options to filled cart and empty cart * append instead of replcae * import style.scss in frontend Co-authored-by: Mike Jolley <[email protected]> Co-authored-by: Raluca Stan <[email protected]>
* move empty cart * remove Cart and rename Cart i2 to Cart * graduate blocks * setup template migration from Cart i1 to Cart i2 * back to js so we have a good diff * add migration * fix bug in empty cart template * add useForceLayout hook to edit * migrate from old block to new block * migrate styles * respect align * add tests * Include latest cart line item improvements from cart-i1 * Missing changes from cart-i1 * Line items table should be disabled * Fix e2e tests for cart i2 * update tests to adapt for inner blocks * update select to resolveSelect to remove warning checker * rename test/block to test/index * move blocks to their own file * undo rename to keep diff clean * remove .tsx and update jest config * Revert "update select to resolveSelect to remove warning checker" This reverts commit 79d55de. * revert resolveControl * Fix empty cart editor E2E test by scrolling to the view switch * parse attributes for order summary block * migrate attributes when resaving * Update documentation Automatic update after running npm run build:docs * add align options to filled cart and empty cart * append instead of replcae * import style.scss in frontend Co-authored-by: Mike Jolley <[email protected]> Co-authored-by: Raluca Stan <[email protected]>
This PR migrates Cart i1 to Cart i2.
Closes #4798
Testing steps
Migration process:
Cart in general:
Mini Cart:
Dev note
Changelog