#2896815: Replace the order summary view with a Twig template has landed and demonstrated two things:
1) Twig is more performant and easier to customize for these use cases
2) We can still support using a View
So, let's apply the same recipe to the cart block view.

CommentFileSizeAuthor
#13 2906024-13.patch7.19 KBThomas Cys
#12 2906024-12.patch7.19 KBThomas Cys
#8 cart_block_simple-2906024-8.patch7.24 KBwaspper
#5 2906024-5.patch7.12 KBmglaman
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bojanz created an issue. See original summary.

Thomas Cys’s picture

Thomas Cys
Primary language Dutch
CreditAttribution: Thomas Cys as a volunteer commented

Added PR for this issue: https://github.com/drupalcommerce/commerce/pull/784

Thomas Cys’s picture

Thomas Cys
Primary language Dutch
CreditAttribution: Thomas Cys as a volunteer commented
Status: Active » Needs review
Thomas Cys’s picture

Thomas Cys
Primary language Dutch
CreditAttribution: Thomas Cys as a volunteer commented
Assigned: Unassigned » Thomas Cys
Status: Needs review » Needs work
mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
Assigned: Thomas Cys » mglaman
FileSize
2906024-5.patch7.12 KB

Rebased patch against HEAD. Reviewing tests, which I forgot I was blocker on for this.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
Status: Needs work » Needs review
bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Status: Needs review » Needs work 
diff --git a/modules/cart/templates/commerce-cart-order-items-table.html.twig b/modules/cart/templates/commerce-cart-order-items-table.html.twig
new file mode 100644
index 00000000..4aa0be66
--- /dev/null
+++ b/modules/cart/templates/commerce-cart-order-items-table.html.twig
@@ -0,0 +1,11 @@
+<table>
+    <tbody>
+    {% for order_item in order_entity.getItems %}
+        <tr>
+            <td>{{ order_item.getQuantity|number_format }} x</td>
+            <td>{{ order_item.label }}</td>
+            <td>{{ order_item.getTotalPrice|commerce_price_format }}</td>
+        </tr>
+    {% endfor %}
+    </tbody>
+</table>
\ No newline at end of file

There are a few problems here:

1) Missing wrapper div that would allow CSS targetting (probably the same wrapper we use for the view?)
2) Unexpected template name. We generally name templates after the role they have, not the markup they contain. So this doesn't match what we did with the checkout order summary, for example. I'd expect the template to be called commerce-cart-block.html.twig
3) Not following coding standards (4 spaces instead of 2, lack of ending newline)
4) Lack of the usual docs block on top

waspper’s picture

waspper CreditAttribution: waspper as a volunteer and at Skilld commented
FileSize
cart_block_simple-2906024-8.patch7.24 KB

Reviewed #5, and added some changes.

Not sure about advice No. 2 at #7, because template is provided for block content. Whole block will remain using the "commerce_cart_block" theme, because adding some content. Therefore, template was named to "commerce-cart-block-content-simple", to match better with its purpose. What do you think? :)

waspper’s picture

waspper CreditAttribution: waspper as a volunteer and at Skilld commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: cart_block_simple-2906024-8.patch, failed testing. View results

Thomas Cys’s picture

Thomas Cys
Primary language Dutch
CreditAttribution: Thomas Cys as a volunteer commented
Assigned: mglaman » Thomas Cys
Thomas Cys’s picture

Thomas Cys
Primary language Dutch
CreditAttribution: Thomas Cys as a volunteer commented
FileSize
2906024-12.patch7.19 KB
Thomas Cys’s picture

Thomas Cys
Primary language Dutch
CreditAttribution: Thomas Cys as a volunteer commented
FileSize
2906024-13.patch7.19 KB
Thomas Cys’s picture

Thomas Cys
Primary language Dutch
CreditAttribution: Thomas Cys as a volunteer commented
Assigned: Thomas Cys » Unassigned
Status: Needs work » Needs review

Don't mind #12
Thanks for the patch waspper.
I just renamed the new template file from commerce-cart-block-content-simple to commerce-cart-block-content.

agoradesign’s picture

agoradesign CreditAttribution: agoradesign commented

When touching the CartBlock class already, we should take the chance and clean up cache metadata.

I've just found some needless lines:

Within build(), a CacheableMetadata object is created and populated, but never attached anywhere.

Further I'm unsure whether the cache context added directly to the render array is redundant - we already have implemented getCacheContexts(). I never needed to define this twice in any block I've created so far.

@bojanz: if this should go into a separate issue, tell me, I can provide a patch (didn't do it because it would only break this one, if it gets sooner committed)

charlieweb82’s picture

charlieweb82 CreditAttribution: charlieweb82 at Acro Commerce commented
Status: Needs review » Needs work

Testing the patch and the template reference commerce-cart-block-content.html.twig in the code don’t load any changes like add a class or delete elements from the template, should be nice to load different field separately, also not use table in a small space. view cart should use in the template div to display like table.

Thomas Cys’s picture

Thomas Cys
Primary language Dutch
CreditAttribution: Thomas Cys as a volunteer commented
Status: Needs work » Needs review

Not 100% sure what you mean by

the code don’t load any changes like add a class or delete elements from the template, should be nice to load different field separately

Remember that this twig file only mimics the view (commerce_cart_block) that is provided by the commerce_cart module. The twig file in this patch should not vary from that.