Use real previews for the Cart and Checkout blocks #2992
Conversation
senadir
added
block: cart
|
Size Change: +51 B (0%) Total Size: 1.66 MB
ℹ️ View Unchanged
|
| @@ -1,3 +1,9 @@ | |||
| .wc-block-cart__page-notice { | |||
| margin: 0; | |||
| } | |||
|
|
|||
| .wp-block-woocommerce-cart.is-editor-preview { | |||
| max-height: 1000px; | |||
There was a problem hiding this comment.
This is a hack to limit the preview height, I filled an issue in GB
WordPress/gutenberg#24484
There was a problem hiding this comment.
If we could display the desktop preview instead of the responsive mobile preview we would probably not have this problem, right? In addition, displaying the mobile preview in a desktop seems a bit weird.
One idea would be to extend the EditorProvider with an isPreview property. This way, inside ContainerWidthContextProvider we could check if it's a preview and, in that case, always apply the isLarge class. I don't think that should block this PR but if it's a solution that makes sense to you too, we could fill an issue.
|
Just noting that we need to test this change across the various versions of WordPress Blocks support (currently WP 5.2+). I think we could consider bumping the next release of Blocks to support WP 5.3+ too if needed and it would simplify any necessary progressive enhancement. |
|
Will test it in other versions, but this shouldn't impact anything because we're not touching anything new, preview attribute has been there since forever. |
|
@senadir @nerrad while I appreciate the idea behind the preview for the styles, I don't think they're necessary in this case. The default, light inputs, are designed to work on all background colors - so this is really optional. Also, the nature of the toggle is such that activating it will provide instant feedback in the editor. If the look isn't desired, it can simply be toggled off. For this reason I think we should stick with a simple toggle switch: |
|
@LevinMedia I agree, but I don't think that should be a blocker for this PR, but more for #2981, I think we can merge this as a good enhancement to our blocks. |
There was a problem hiding this comment.
This is looking great, previews make more sense than just showing a gray SVG placeholder. I wonder why went for the SVG placeholder in the first place. 🤔
For this reason I think we should stick with a simple toggle switch:
So, if I'm understanding it right, if we use the toggle instead of the style previews, this should be removed from the testing steps Make sure the styles are not very long and overflowing, right? Or at least, re-phrased so it refers to the inserter preview instead of the styles.
I left some comments below, but pre-approving. I think it would be good to handle the comments about removing some dead files and a couple of lines in CSS in this PR before merging. The other one about investigating how we could display desktop previews in the inserter shouldn't be blocking in my opinion.
| @@ -1,3 +1,9 @@ | |||
| .wc-block-cart__page-notice { | |||
| margin: 0; | |||
| } | |||
|
|
|||
| .wp-block-woocommerce-cart.is-editor-preview { | |||
| max-height: 1000px; | |||
There was a problem hiding this comment.
If we could display the desktop preview instead of the responsive mobile preview we would probably not have this problem, right? In addition, displaying the mobile preview in a desktop seems a bit weird.
One idea would be to extend the EditorProvider with an isPreview property. This way, inside ContainerWidthContextProvider we could check if it's a preview and, in that case, always apply the isLarge class. I don't think that should block this PR but if it's a solution that makes sense to you too, we could fill an issue.
Part of #2981
This PR uses real previews for the cart and checkout, something we can use because:
1- We have container queries so our blocks are responsive regardless of the screen size.
2- We don't have a select product step for those blocks, what you see is what you get.
Screenshots
Before:
After:
How to test the changes in this Pull Request:
Changelog