Problem/Motivation

The 7.x version shows a confirmation popup (via ajax) after you add a porduct to cart. Do something similar in 8.x as well. This would be a sweet feature of this module.

Proposed resolution

There are loads of options/ideas available here:

Pick one of them, and integrate it with this module. DON'T add anything of your own, as I did in 7.x

Decided to write a sub-module for this feature, and not use the options mentioned above. Reasons:

This feature should be added as a sub-module. Reason: People may/may-not want the popup confirmation by default. Commerce Kickstart has the ability to disable the popup confirmation, and I want that ability in this module.

Remaining tasks

N/A

User interface changes

Not yet decided.

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#27 2910744-27.patch12.81 KBbrockfanning
#22 interdiff-15-22.txt7.33 KBsubhojit777
#22 2910744-22.patch13.48 KBsubhojit777
#15 dc_ajax_add_cart-popup-2910744-16.patch10.91 KBbrockfanning
#15 dc_ajax_add_cart-popup-interdiff-2910744-8-16.txt8.43 KBbrockfanning
#14 dc_ajax_add_cart-popup-interdiff-2910744-8-14.txt8.25 KBbrockfanning
#14 dc_ajax_add_cart-popup-2910744-14.patch10.95 KBbrockfanning
#12 dc_ajax_add_cart-popup-interdiff-2910744-6-12.txt7.51 KBbrockfanning
#12 dc_ajax_add_cart-popup-2910744-12.patch10.21 KBbrockfanning
#8 dc_ajax_add_cart-popup-2910744-8.patch9.39 KBbrockfanning
#6 dc_ajax_add_cart-popup-2910744-6.patch9.15 KBbrockfanning
#3 dc_ajax_add_cart-popup-2910744.patch2.06 KBbrockfanning
#2 first-step-but-not-complete.patch441 bytesbrockfanning
confirmation-popup.png13.78 KBsubhojit777

Comments

subhojit777 created an issue. See original summary.

brockfanning’s picture

brockfanning commented
Priority: Normal » Major
StatusFileSize
new first-step-but-not-complete.patch441 bytes

First off, this module looks REALLY promising and well-maintained! I'm looking forward to helping out, if needed.

Personally I think this popup behavior is a major requirement, because of the fact that AJAX is typically used to avoid changes in page scroll. If the AJAX itself causes the page to scroll to the top, this benefit is lost. In fact most users/clients will probably assume the page refreshed completely.

So I think it's important to get away from status messages and implement this popup functionality. I'm attaching an example patch (not intended to be reviewed) of the first step: removing the scroll change. I'll start thinking about the popup implementation next.

brockfanning’s picture

brockfanning commented
StatusFileSize
new dc_ajax_add_cart-popup-2910744.patch2.06 KB

Here's a bit more progress - putting the status message into a modal. Still a lot to do though, like implementing the popup with a Twig template.

subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented

Re: #2:

AJAX is typically used to avoid changes in page scroll. If the AJAX itself causes the page to scroll to the top, this benefit is lost.

This is a good UX feature that you mentioned.

So I think it's important to get away from status messages and implement this popup functionality.

Showing the status messages is necessary. While working in 7.x version of this module, people complained that this module does not shows feedback messages, or error messages. So, I do not want to lose this feature in 8.x.

If you still do not want the status messages, you can either fork this module https://github.com/subhojit777/dc_ajax_add_cart, or you can create a new issue and upload a patch there and use it with https://github.com/cweagans/composer-patches.

Re #3:

Thank you for taking the lead :) Some feedback:

  • Could you do this in a separate module. Because people may/may-not want the popup by default. Commerce Kickstart has the ability to stop the popup, and I want to retain that feature.
subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented
Issue summary: View changes
brockfanning’s picture

brockfanning commented
Status: Active » Needs review
StatusFileSize
new dc_ajax_add_cart-popup-2910744-6.patch9.15 KB

Here's an attempt at this submodule, which also adds a new view mode for product variations, and a Twig template for the popup.

Status: Needs review » Needs work

The last submitted patch, 6: dc_ajax_add_cart-popup-2910744-6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

brockfanning’s picture

brockfanning commented
Status: Needs work » Needs review
StatusFileSize
new dc_ajax_add_cart-popup-2910744-8.patch9.39 KB

Testing fixes.

subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented

Thank you :) I will test the patch. We will need tests of course. I will update this issue with the test cases and feedback. Thank you.

subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented

Some observations (mostly nitpiks):

  1. +++ b/modules/dc_ajax_add_cart_popup/README.md
@@ -0,0 +1,10 @@
+- Go to `admin/commerce/config/product-variation-types/default/edit/display`, check
+  **Ajax add to cart popup** under `Custom Display Settings`, and click `Save`.

    Could you please add this instructions in the parent module's README. This should be added under Installation and usage of the parent README. 

  2. +++ b/modules/dc_ajax_add_cart_popup/dc_ajax_add_cart_popup.info.yml
@@ -0,0 +1,7 @@
+  - dc_ajax_add_cart

    s/dc_ajax_add_cart/drupal:dc_ajax_add_cart. Sometimes coder throws error if not done like that. 

  3. +++ b/modules/dc_ajax_add_cart_popup/dc_ajax_add_cart_popup.module
@@ -0,0 +1,34 @@
+        'product_variation_label' => NULL,

    Not required. We are passing the whole entity. 

  4. +++ b/modules/dc_ajax_add_cart_popup/dc_ajax_add_cart_popup.module
@@ -0,0 +1,34 @@
+function dc_ajax_add_cart_popup_form_alter(&$form, &$form_state, $form_id) {

    I have a feeling that we can replace hook_form_alter() with a more specific hook_form_BASE_FORM_ID_alter (). And remove the inner if condition. 

  5. +++ b/modules/dc_ajax_add_cart_popup/src/EventSubscriber/AjaxAddToCartPopupSubscriber.php
@@ -0,0 +1,82 @@
+    $view_builder = \Drupal::entityManager()->getViewBuilder('commerce_product_variation');

    Use depedency injection instead. 

  6. +++ b/modules/dc_ajax_add_cart_popup/src/EventSubscriber/AjaxAddToCartPopupSubscriber.php
@@ -0,0 +1,82 @@
+      '#product_variation_label' => $this->purchasedEntity->label(),

    We don't need this. We are already passing the full entity. 

  7. +++ b/modules/dc_ajax_add_cart_popup/src/EventSubscriber/AjaxAddToCartPopupSubscriber.php
@@ -0,0 +1,82 @@
+    $title = '';

    Remove this. This is not used anywhere. 

  8. +++ b/modules/dc_ajax_add_cart_popup/src/EventSubscriber/AjaxAddToCartPopupSubscriber.php
@@ -0,0 +1,82 @@
+    $options = ['width' => '700'];

    Ideally this should be coming from an admin setting. But not to worry now. We can do that later, in a separate issue. 

  9. +++ b/modules/dc_ajax_add_cart_popup/tests/src/FunctionalJavascript/AjaxAddCartPopupTest.php
@@ -0,0 +1,57 @@
+    'commerce_product',
+    'commerce_cart',
+    'dc_ajax_add_cart',
+    'dc_ajax_add_cart_popup',

    Just do dc_ajax_add_cart_popup. Others will be enabled as dependent modules. 

  10. +++ b/modules/dc_ajax_add_cart_popup/tests/src/FunctionalJavascript/AjaxAddCartPopupTest.php
@@ -0,0 +1,57 @@
+  protected $profile = 'standard';

    Have you checked running this in the default testing profile. 

  11. +++ b/modules/dc_ajax_add_cart_popup/tests/src/FunctionalJavascript/AjaxAddCartPopupTest.php
@@ -0,0 +1,57 @@
+  public function testPopup() {

    AWESOME!! AWESOME!! This is great. Thank you for adding the tests without being asked.

    Sorry for my last comment. I just skimmed the patch last time, didn't checked it fully. 

  12. +++ b/modules/dc_ajax_add_cart_popup/tests/src/FunctionalJavascript/AjaxAddCartPopupTest.php
@@ -0,0 +1,57 @@
+    $this->waitForAjaxToFinish();

    I have also added negative tests. Add another test that would check whether the popup is indeed ajaxified. I check that by not calling $this->waitForAjaxToFinish().

I would say that your work is almost perfect :) 👌 Thank you Brock.

subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented
Status: Needs review » Needs work

Some observations (mostly nitpiks):

  1. +++ b/modules/dc_ajax_add_cart_popup/README.md
@@ -0,0 +1,10 @@
+- Go to `admin/commerce/config/product-variation-types/default/edit/display`, check
+  **Ajax add to cart popup** under `Custom Display Settings`, and click `Save`.

    Could you please add this instructions in the parent module's README. This should be added under Installation and usage of the parent README. 

  2. +++ b/modules/dc_ajax_add_cart_popup/dc_ajax_add_cart_popup.info.yml
@@ -0,0 +1,7 @@
+  - dc_ajax_add_cart

    s/dc_ajax_add_cart/drupal:dc_ajax_add_cart. Sometimes coder throws error if not done like that. 

  3. +++ b/modules/dc_ajax_add_cart_popup/dc_ajax_add_cart_popup.module
@@ -0,0 +1,34 @@
+        'product_variation_label' => NULL,

    Not required. We are passing the whole entity. 

  4. +++ b/modules/dc_ajax_add_cart_popup/dc_ajax_add_cart_popup.module
@@ -0,0 +1,34 @@
+function dc_ajax_add_cart_popup_form_alter(&$form, &$form_state, $form_id) {

    I have a feeling that we can replace hook_form_alter() with a more specific hook_form_BASE_FORM_ID_alter (). And remove the inner if condition. 

  5. +++ b/modules/dc_ajax_add_cart_popup/src/EventSubscriber/AjaxAddToCartPopupSubscriber.php
@@ -0,0 +1,82 @@
+    $view_builder = \Drupal::entityManager()->getViewBuilder('commerce_product_variation');

    Use depedency injection instead. 

  6. +++ b/modules/dc_ajax_add_cart_popup/src/EventSubscriber/AjaxAddToCartPopupSubscriber.php
@@ -0,0 +1,82 @@
+      '#product_variation_label' => $this->purchasedEntity->label(),

    We don't need this. We are already passing the full entity. 

  7. +++ b/modules/dc_ajax_add_cart_popup/src/EventSubscriber/AjaxAddToCartPopupSubscriber.php
@@ -0,0 +1,82 @@
+    $title = '';

    Remove this. This is not used anywhere. 

  8. +++ b/modules/dc_ajax_add_cart_popup/src/EventSubscriber/AjaxAddToCartPopupSubscriber.php
@@ -0,0 +1,82 @@
+    $options = ['width' => '700'];

    Ideally this should be coming from an admin setting. But not to worry now. We can do that later, in a separate issue. 

  9. +++ b/modules/dc_ajax_add_cart_popup/tests/src/FunctionalJavascript/AjaxAddCartPopupTest.php
@@ -0,0 +1,57 @@
+    'commerce_product',
+    'commerce_cart',
+    'dc_ajax_add_cart',
+    'dc_ajax_add_cart_popup',

    Just do dc_ajax_add_cart_popup. Others will be enabled as dependent modules. 

  10. +++ b/modules/dc_ajax_add_cart_popup/tests/src/FunctionalJavascript/AjaxAddCartPopupTest.php
@@ -0,0 +1,57 @@
+  protected $profile = 'standard';

    Have you checked running this in the default testing profile. 

  11. +++ b/modules/dc_ajax_add_cart_popup/tests/src/FunctionalJavascript/AjaxAddCartPopupTest.php
@@ -0,0 +1,57 @@
+  public function testPopup() {

    AWESOME!! AWESOME!! This is great. Thank you for adding the tests without being asked.

    Sorry for my last comment. I just skimmed the patch last time, didn't checked it fully. 

  12. +++ b/modules/dc_ajax_add_cart_popup/tests/src/FunctionalJavascript/AjaxAddCartPopupTest.php
@@ -0,0 +1,57 @@
+    $this->waitForAjaxToFinish();

    I have also added negative tests. Add another test that would check whether the popup is indeed ajaxified. I check that by not calling $this->waitForAjaxToFinish().

I would say that your work is almost perfect :) 👌 Thank you Brock.

brockfanning’s picture

brockfanning commented
Status: Needs work » Needs review
StatusFileSize
new dc_ajax_add_cart-popup-2910744-12.patch10.21 KB
new dc_ajax_add_cart-popup-interdiff-2910744-6-12.txt7.51 KB

Thanks for the feedback. Here's an attempt to hit all those items.

Status: Needs review » Needs work

The last submitted patch, 12: dc_ajax_add_cart-popup-2910744-12.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

brockfanning’s picture

brockfanning commented
Status: Needs work » Needs review
StatusFileSize
new dc_ajax_add_cart-popup-2910744-14.patch10.95 KB
new dc_ajax_add_cart-popup-interdiff-2910744-8-14.txt8.25 KB

Not sure what's going on with the tests - locally I get occasional/scattered failures but not so many as above. Hopefully it was a fluke, and this time will be better. Also I noticed I forgot the README change, so here is another try with an updated patch.

brockfanning’s picture

brockfanning commented
StatusFileSize
new dc_ajax_add_cart-popup-interdiff-2910744-8-16.txt8.43 KB
new dc_ajax_add_cart-popup-2910744-16.patch10.91 KB

Almost there - a couple of code-sniff fixes.

subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented

Thank you :) I will take a look.

subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented

Thanks. Some more nitpiks and test improvements we could do:

  1. +++ b/modules/dc_ajax_add_cart_popup/README.md
@@ -0,0 +1,10 @@
+The is a submodule of Commerce Ajax Add to Cart. This displays a modal popup after

    s/The is a sumodule/A submodule. 

  2. +++ b/modules/dc_ajax_add_cart_popup/src/EventSubscriber/AjaxAddToCartPopupSubscriber.php
@@ -0,0 +1,101 @@
+    if (!$this->purchasedEntity) {

    We should write a test for this. 

  3. +++ b/modules/dc_ajax_add_cart_popup/src/EventSubscriber/AjaxAddToCartPopupSubscriber.php
@@ -0,0 +1,101 @@
+    if (!$response instanceof AjaxResponse) {

    We should write a test for this. 

  4. +++ b/modules/dc_ajax_add_cart_popup/tests/src/FunctionalJavascript/AjaxAddCartPopupTest.php
@@ -0,0 +1,59 @@
+    // Confirm that the initial add to cart submit works.
+    $this->cart = Order::load($this->cart->id());
+    $order_items = $this->cart->getItems();
+    $this->assertOrderItemInOrder($this->variation, $order_items[0]);

    Will not be required. We are already testing in the parent module's test. 

  5. +++ b/modules/dc_ajax_add_cart_popup/tests/src/FunctionalJavascript/AjaxAddCartPopupTest.php
@@ -0,0 +1,59 @@
+    $this->assertSession()->pageTextContains("The item has been added to your cart.", 'Popup not found.');

    This is okay. But it would be great, if we test this backend-wise as well. Like, check if the current response contains OpenModalDialogCommand. 

  6. +++ b/modules/dc_ajax_add_cart_popup/tests/src/FunctionalJavascript/AjaxAddCartPopupTest.php
@@ -0,0 +1,59 @@
+    $this->assertSession()->pageTextNotContains("The item has been added to your cart.", 'Popup was found before AJAX finished.');

    This is okay. But it would be great, if we test this backend-wise as well. Like, check if the current response contains OpenModalDialogCommand.

Thanks for the effort Brock. I was looking into the bugs that were reported few days ago.

The patch looks perfect. I think we can use this idea in your patch to fix #2930926: Cart block should be updated (ajax) when item removed from cart. I will try to do the fixes next week, and do a manual check on my local. Thank you.

brockfanning’s picture

brockfanning commented

Hi @subhojit777, cool. Let me know if there's anything else I can help with.

subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented
Issue summary: View changes
subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented
Issue summary: View changes
subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented
Issue summary: View changes
subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented
StatusFileSize
new 2910744-22.patch13.48 KB
new interdiff-15-22.txt7.33 KB
subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented

Hey Brock,

Could you please review the patch in #22. I have added some notes below.

  1. +++ b/modules/dc_ajax_add_cart_popup/src/EventSubscriber/AjaxAddToCartPopupSubscriber.php
@@ -0,0 +1,98 @@
+    if (!$this->purchasedEntity) {

    This if is covered in AjaxAddCartPopupTest::testAjaxPopup() that you wrote. We are good here. 

  2. +++ b/modules/dc_ajax_add_cart_popup/tests/src/FunctionalJavascript/AddCartPopupAjaxResponseTest.php
@@ -0,0 +1,45 @@
+  public function testPopupOnAjaxResponse() {

    I have added this test which is going to check that the popup appears only if a product is added to cart via ajax, i.e. this if condition:

    if (!$response instanceof AjaxResponse) {

    It also had the check whether the product is added to cart. So that means, it checks that product is successfuly aded to cart, while the popup does not appears. 

  3. +++ b/modules/dc_ajax_add_cart_popup/tests/src/FunctionalJavascript/AjaxAddCartPopupTest.php
@@ -0,0 +1,59 @@
+    // Confirm that the initial add to cart submit works.
+    $this->cart = Order::load($this->cart->id());
+    $order_items = $this->cart->getItems();
+    $this->assertOrderItemInOrder($this->variation, $order_items[0]);

    In #17.4 I asked you to remove this. But later I realized that it should be here. Because it checks AjaxAddToCartPopupSubscriber's if (!$this->purchasedEntity). i.e. popup appears ONLY when an item is added to cart. 

  4. +++ b/modules/dc_ajax_add_cart_popup/tests/src/FunctionalJavascript/AjaxAddCartPopupTest.php
@@ -0,0 +1,59 @@
+    $this->assertSession()->pageTextContains("The item has been added to your cart.", 'Popup not found.');

    In #17.5 I asked for a backend check, i.e. whether OpenModalDialogCommand is indeed present. That check is going to be tricky here. Mink does not keeps track of the type of response.

    I also compared this with Commerce core. They are doing something similar to check event subscribers.

    Lets keep it this way. Simple. Later we can add a backend check test if there is a problem.

subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented
n3xo’s picture

n3xo commented

Any ideas on how to display the cart directly in the pop-up? So I think the user experience should improve!

subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented

You need to show the cart itself in the popup everytime product is added to cart?

+++ b/modules/dc_ajax_add_cart_popup/src/EventSubscriber/AjaxAddToCartPopupSubscriber.php
@@ -0,0 +1,98 @@
+    $view_builder = $this->entityTypeManager->getViewBuilder('commerce_product_variation');
+    $product_variation = $view_builder->view($this->purchasedEntity, 'dc_ajax_add_to_cart_popup');
+    $content = [
+      '#theme' => 'dc_ajax_add_cart_popup',
+      '#product_variation' => $product_variation,
+      '#product_variation_entity' => $this->purchasedEntity,
+      '#cart_url' => Url::fromRoute('commerce_cart.page')->toString(),
+    ];
+    $title = '';
+    $options = ['width' => '700'];
+    $response->addCommand(new OpenModalDialogCommand($title, $content, $options));
+    $event->setResponse($response);

You can take the idea from here.

You have to pass the cart block markup in the $content. You can check the existing module code. You will find the code to programmatically render the block. You have glue them together.

brockfanning’s picture

brockfanning commented
StatusFileSize
new 2910744-27.patch12.81 KB

@subhojit777: Everything looks great to me, except the change to .travis.yml is keeping it from applying. Here's a version without that change, in case it's helpful.

subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented

No that's okay. I changed the Travis settings a bit, otherwise it was failing for readme. Thanks for the review. I will push it and make a new release.

Status: Needs review » Needs work

The last submitted patch, 27: 2910744-27.patch, failed testing. View results

brockfanning’s picture

brockfanning commented
Status: Needs work » Reviewed & tested by the community

subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented
Status: Reviewed & tested by the community » Fixed

@brockfanning Thanks for the help :)

subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented

This is now available as beta1 release.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.