I have installed Commerce Ajax Commerce 7.x-1.x-dev version, and it doesn't work by default.
So I made count of rework, that should help for non-developers to start using this module, more useful README.txt created.
Please review, temporary demo of this rework located on http://demo.nikiit.ru/node/5 - Drupal with Commerce (no Commerce Kit).

Comments

Nikit’s picture

Issue summary: View changes
Nikit’s picture

Adding multiple parent class and Ids.

Status: Needs review » Needs work

The last submitted patch, 2: classids_support-2448257.patch, failed testing.

Nikit’s picture

Reapplying changes.

joelpittet’s picture

Status: Needs work » Needs review

There are a few unnecessary changes in there like the rename of the readme file from markdown to text. And the "images" folder to "theme" folder. Though there are a lot of great things in this as well.

Can you remove or move those unnecessary changes to another issue and fix the whitespace issues with trailing spaces?

joelpittet’s picture

Also thank you taking the time to fixing those issues in the dev branch!

Nikit’s picture

FileSize
42.05 KB

thanks for comment, Joël!

Why I made these:
1. readme.txt used by "Advanced Help" module, and textual extension more closer for users, than MarkDown (even I like it);
2. images folder was removed for trying reducing count of folders. "Theme" folder, I assume, has the same functionality.

At this time I confirm your changes and add changes on commit. Please check...

Status: Needs review » Needs work

The last submitted patch, 7: commerce_ajax_cart-rework-2448257-9713191.patch, failed testing.

joelpittet’s picture

theme folder is a bit of a misnomer, it should be hold tpl files and IMO should be called templates, but that should be for another issue. I've not used "Advanced Help", and their documentation or project page doesn't mention any conventions to follow... can you point to where this file extension is recommended by them?

Here's a more thorough code review:

  1. +++ b/README.md
    @@ -1,9 +1,98 @@
    +---------------------
    +  ¶
    +* About Commerce Ajax Cart
    ...
    +1. Visit and enable it: admin/structure/views. ¶
    
    +++ b/commerce_ajax_cart.module
    @@ -199,18 +243,45 @@ function commerce_ajax_cart_callback($form, &$form_state) {
    +  ¶
       $commands[] = array('command' => 'commerce_ajax_cart_update');
    -  $commands[] = ajax_command_prepend('#block-system-main', theme('status_messages'));
    
    +++ b/js/commerce_ajax_cart.js
    @@ -86,10 +80,53 @@
    +        );
    +        // Break cycle if found and animated. ¶
    +        return false;
    

    Extra space at the end of line. You can configure your editor/ide to automatically trim these on save to prevent these from happening.
    Example if you use sublime text: https://www.drupal.org/node/1346890 or PHPStorm https://www.drupal.org/node/1962108

  2. +++ b/commerce_ajax_cart.info
    @@ -4,10 +4,5 @@ package = Commerce (contrib)
    -stylesheets[all][] = css/commerce_ajax_cart.css
    -; Information added by Drupal.org packaging script on 2014-01-17
    -version = "7.x-1.0-beta2"
    -core = "7.x"
    -project = "commerce_ajax_cart"
    -datestamp = "1389966808"
    

    This info is from a real download not the git repo that's why the patch is failing I believe.

  3. +++ b/commerce_ajax_cart.module
    @@ -100,38 +114,28 @@ function commerce_ajax_validate_post_request() {
    -  drupal_add_library('system', 'ui.position', 'drupal.ajax');
    +  drupal_add_library('system', 'ui.position');
    +  drupal_add_library('system', 'drupal.ajax');
    

    This is a great catch thank you for fixing this!

  4. +++ b/commerce_ajax_cart.module
    @@ -199,18 +243,45 @@ function commerce_ajax_cart_callback($form, &$form_state) {
    -    '#commands' => $commands,
    +    '#commands' => $commands
    

    This change isn't needed as is incorrect, always put a comma at the end of array items as per the coding standards.
    https://www.drupal.org/coding-standards#array

  5. +++ b/js/commerce_ajax_cart.js
    @@ -86,10 +80,53 @@
    +        }).appendTo(jQuery('body')).animate({
    

    jQuery is in a the closure so you can use $

  6. +++ b/js/commerce_ajax_cart.js
    @@ -86,10 +80,53 @@
    +
    ...
    +
    +
    ...
    +
    

    extra line breaks not needed.

  7. +++ b/theme/commerce-ajax-cart-dialog.tpl.php
    @@ -0,0 +1,26 @@
    +    <a href="<?php echo url('cart'); ?>" class="cart-link"><?php echo t('Go to Cart'); ?></a>
    +    <a href="<?php echo url('checkout'); ?>" class="checkout-link"><?php echo t('Go to Checkout'); ?></a>
    

    print instead of echo for consistency. (though I like echo better it's not the drupal way)

  8. +++ b/theme/commerce-ajax-cart.tpl.php
    @@ -0,0 +1,22 @@
    +</a>
    \ No newline at end of file
    

    Need a newline at end of file.

Install dreditor in your browser so you can see some of these things easier. http://dreditor.org/

Nikit’s picture

FileSize
42.02 KB

whitespaces and LF fixes...

joelpittet’s picture

@Nikit to fast track a bit of this, if you don't mind, I'll commit all the safe changes that I can and post a new patch with your remaining changes so you can continue to refine those bits. Does that work for you? I'll wait till you next cleanup of some of the items in #9

Nikit’s picture

FileSize
27.83 KB

1. Fixed
2. Not sure, why packaging info stored in git clone. I assume, it appear automatically when Drupal.org prepare for project archived file.
4. Fixed
5. Fixed, also missed one.
6. Fixed, empty rows reduced.
7. "echo" replaced "print", I prefer "print", since echo for me just console echo-ing: old modem "bulleting board systems" behaviour :)
8. Fixed

Due to comment, yes, I can reapply my changes after your patches.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
16.69 KB

@Nikit somehow the view changes got lost between #10 and #12

I've committed the low impact changes, we can now concentrate on the bulk of the rest of this patch.

Here is your patch from #12 remaining bits to review and commit.

  • joelpittet committed 3cfb2ff on 7.x-1.x authored by Nikit
    Issue #2448257 by Nikit, joelpittet: Commerce Ajax Cart rework
    
Nikit’s picture

Thanks for patching.

I didn't change commerce_ajax_cart.views_default.inc, so no patches should be...

joelpittet’s picture

Status: Needs review » Needs work

Here's another review:

  1. +++ b/commerce_ajax_cart.admin.inc
    @@ -52,42 +52,61 @@ function commerce_ajax_cart_settings_form() {
    +  $current_theme = variable_get('theme_default', 'garland');
    +  $blocks = block_admin_display_prepare_blocks($current_theme);
    

    Can we get away with not loading garland to get the block list?

  2. +++ b/commerce_ajax_cart.admin.inc
    @@ -52,42 +52,61 @@ function commerce_ajax_cart_settings_form() {
    +    if ($block['module'] == 'commerce_cart') {
    +      $blocks_list[$block['bid']] = $block['info'];
    +    }
    

    Should we open this up so that people could use whichever block they wished?

  3. +++ b/commerce_ajax_cart.admin.inc
    @@ -52,42 +52,61 @@ function commerce_ajax_cart_settings_form() {
    +    '#title' => t('Content id or class'),
    

    Can this be any CSS selector?

  4. +++ b/commerce_ajax_cart.admin.inc
    @@ -52,42 +52,61 @@ function commerce_ajax_cart_settings_form() {
    +  $form['commerce_ajax_cart_fly2cart'] = array(
    ...
    +  // TODO Bad. Better solution?
    +  $form['commerce_ajax_cart_fly2cart_parent'] = array(
    

    Maybe this can be moved to another issue?

  5. +++ b/js/commerce_ajax_cart.js
    @@ -32,7 +32,7 @@
    -          $('a.commerce-ajax-cart-loader').html(data);
    +          $('a.commerce-ajax-cart-loader').parent().html(data);
    

    Why was this parent() needed? We can probably remove the a tag from the selector.

joelpittet’s picture

About the view missing, sorry I just noticed in the patch something changed in the view but maybe that was a whitespace change you can see it in the patch with dreditor and the patch size being 42kb vs 27kb in #12

Nikit’s picture

FileSize
21.51 KB

1. Code for getting default theme blocks, not garland. But I change it to the variable_get('theme_default', 'bartik');.

2 Users cann't select any block there, only cart module block. In commerce_ajax_cart_preprocess_block function next code used:

  if ('commerce_cart' == $variables['block']->module && $variables['block_id'] == 1) {

- but block_id isn't always equal to 1. It's increment sometimes, for example, reinstalling commerce cart module.
I assume, making only ('commerce_cart' == $variables['block']->module) also incorrect, since in page there can be 2 or more cart blocks (top&bottom, left&right).

3. Yes, depend on what wrapper placed statuses in page.tpl.php - it'snt always #block-system-main. Also this setting allow place cart message everywere: header, left/right blocks and so on as user want.

4. It's just marking that there may be better solution for calculation parent of product cart form and image of product. At this moment, I provide this solution, community can provide better, so just marked as TODO everywhere in code.

5. Parent of this always cart block, that was replaced by commerce_ajax_cart_preprocess_block function, so at this time js code broke nothing. But feel free for making it better.

My addings:

6. No need $form['actions']['submit'] in admin settings form, it will added by there system_settings_form($form) automatically.

7. again for README.md - MarkDown format not so known for "usual" users, txt format is more famous. Also Advanced help module (139,155 installations) use it (row #799 in advanced_help.module).
It's not required, just recommendation.

8. commerce_ajax_cart_script_examples is obsolete. It's unuseful for users, developers can read on README.txt or API.txt

9. again for removing images folder. theme folder files, I assume, can be overrided in user theme folder. So users can know that ajax-loader.gif - can be replaced by their own loader, as the same for tpl files. And we reducing folder count.
It's just my mind.

10. ; Information added by Drupal.org packaging script on 2014-01-17 and bottom lines removed from commerce_ajax_cart.info. This info generated by Drupal.org. See generated http://ftp.drupal.org/files/projects/commerce_ajax_cart-7.x-1.x-dev.tar.gz - there this info placed twice.

11. Main goal of remaking this module is making it workable and UI friendly for all, not only for ready-to-use Commerce Kit. At this time this goal acceeded, I assume, 70-80%.

Nikit’s picture

Issue summary: View changes
Nikit’s picture

Issue summary: View changes
Nikit’s picture

Status: Needs work » Needs review

hello Joel! 9 months passed, what about this post/patch?

Status: Needs review » Needs work

The last submitted patch, 18: commerce_ajax_cart-rework-2448257-9717075.patch, failed testing.

berenddeboer’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
21.12 KB

Great work Niikit! Here a reroll of your patch.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 23: commerce_ajax_cart-rework-2448257-23.patch, failed testing.

berenddeboer’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
18.54 KB

Rerolled patch against git, so hopefully applies now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 25: commerce_ajax_cart-rework-2448257-25.patch, failed testing.

The last submitted patch, 25: commerce_ajax_cart-rework-2448257-25.patch, failed testing.

The last submitted patch, 25: commerce_ajax_cart-rework-2448257-25.patch, failed testing.

Nikit’s picture

hello @berenddeboer!
Looks like error happen on test unit, I'll check that as soon as I will free.
Thank you!

dieuwe’s picture

Status: Needs work » Reviewed & tested by the community

It looks like the error is that there are no tests to run? I sure can't find any.

20:11:43 ERROR: No valid tests were specified.

dieuwe’s picture

Just realised that the theme folder is missing from that last patch, it should contain 2 templates (they are in the previous one).

dieuwe’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
20.87 KB

This patch should contain all the right files. I suspect it will still fail CI (due to no tests being found).

Status: Needs review » Needs work

The last submitted patch, 32: commerce_ajax_cart_rework-2448257-32.patch, failed testing.

joelpittet’s picture

Assigned: Nikit » Unassigned
Status: Needs work » Needs review

Tests failed because there are none, resetting status

dieuwe’s picture

Sorry to add another patch, but I wanted to include a "configure" link in the info file so here it is again.

Should we add a test file or just go with what we have?