Problem/Motivation

Follow up of #2643158: AJAX library not added when cart is empty

Right now we are using dc_ajax_add_cart_include_cart_resources() to add CSS, JS resources and AJAX library which are required to render the cart and make it functional.

As Matt suggested here #2643158-10: AJAX library not added when cart is empty, lets use hook_library() to load the resources.

Proposed resolution

Remove dc_ajax_add_cart_include_cart_resources() and use hook_library() to load the resources.

Remaining tasks

N/A

User interface changes

N/A

API changes

Remove dc_ajax_add_cart_include_cart_resources() and use hook_library() to load the resources.

Data model changes

N/A

Comments

subhojit777 created an issue. See original summary.

subhojit777’s picture

joelpittet’s picture

Status: Active » Needs review
StatusFileSize
new2.09 KB

Not sure what to add for some of the things, like what to name the library, I went with 'replace' because that is what it was doing for the command, kinda boring you could probably think of something better;)

subhojit777’s picture

Thank you. Looks good to me. Can we call it main, as that is the main library that makes the ajax cart functional. We could have called it replace, but the CSS, that is not playing any part in "replace"-ing, so I think the name main suites here more :) I am open to more suggestions.

Also I was thinking why dc_ajax_add_cart.js is not part of the library. I checked dc_ajax_add_cart.js, and I decided we can optionally attach that JS as per dc_ajax_add_cart_display_popup setting. We are going to add that check inside dc_ajax_add_cart_form_alter(). We can open a new issue for that. Let me know what you think.

joelpittet’s picture

StatusFileSize
new1.02 KB
new2.08 KB

@subhojit777 yeah 'main' is fine with me, I've seen other's use 'base' too, but it's all a bit of personal preference I believe and makes itself more evident when you add more.

The dc_ajax_add_cart.js looks like it's included in the right place, it could be in it's own library and attached that way but I don't see any really benefits off hand. Maybe a separate issue if you'd like to explore that more.

subhojit777’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

The dc_ajax_add_cart.js looks like it's included in the right place, it could be in it's own library and attached that way but I don't see any really benefits off hand. Maybe a separate issue if you'd like to explore that more.

Makes sense.

The patch is perfect!! Can you please reroll the patch. And while you are doing it, can you please update the library name to base :D

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new2.11 KB
new1.02 KB

No problem, changed to base and re-base-rolled.

subhojit777’s picture

Status: Needs review » Needs work
StatusFileSize
new71.1 KB

Thank you. I am seeing this JS error, just on page load.

Uncaught TypeError: Cannot read property 'prototype' of undefined

joelpittet’s picture

Thanks that is bizarre, the order of the files seems to be wrong

  • subhojit777 committed 706d428 on 7.x-2.x
    Issue #2891050 by joelpittet: Use hook_library() to add resources
    
subhojit777’s picture

Status: Needs work » Fixed

The ordering was right. The issue was with the weight. https://api.drupal.org/api/drupal/modules%21system%21system.module/funct...

subhojit777’s picture

The tests were passing because simpletests are buggy when it comes to testing JS behavior. Ideally the tests should be browser automated tests.

joelpittet’s picture

I would have expected not putting a group would have done JS_DEFAULT, which is the weight of 0. AND because it has a dependency it would order after the thing that's needed... hmmm

subhojit777’s picture

Yeah.. it is a supposed-to-be assumption, but still not :)

Status: Fixed » Closed (fixed)

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