Improve the markup of the checkout review pane.

Quick question - is your GitHub fork based on the d.o repository or does it still use the commit history of the drupalcommerce/drupalcommerce GitHub repository? If it's the old repo history, can I get you to re-post this as a patch?

Before the patch, my review page on Bartik looks like this:

After the patch, it looks like this:

Perhaps I'm the only person in the world, but i find the "table" look (really the lines and variations in color) to be helpful in making sense of the page. After the patch without any other changes, it just appears like some random text with odd bolding and slight size changes. The problem is it's going to be hard to add custom styling that is good for all themes, unless there are common classes we can tap into. : ?

@googletorp Can you add a <hr> below each <h3> ? And post a new screenshot?

I am not sold on this change either, it seems to create less visual hierarchy. It'd be interested what usecase you ran into that made it difficult to move stuff around, I can understand if you for example wish to place shipping information to the right of billing information that it can be hard (which it shouldn't be).

Bartik is not really made for this kind of visual hierarchy, because titles are made for large parts of content, not short blurbs. Adding something like a <hr> could solve that - but I am not sure.

I really don't understand this, why doesn't commerce provide html base? And provide customization for bartik? @googletorp It's definitely not worth a bad themer experience, but lets come to a solution that doesn't require bartik looking ugly.

I think we skewed the discussion in a wrong direction, lets not do that.

I have to second googletorp, I think the change in the markup is required and makes customizing the review stage much simpler.

Idan

I applied the patch and i'm getting these errors:

: include_once(//sites/default/modules/commerce/modules/checkout/theme/includes/commerce_checkout.pages.inc) [function.include-once]: failed to open stream: No such file or directory in theme() (line 794 of /l/httpdocs/includes/theme.inc).
Warning: include_once() [function.include]: Failed opening '/httpdocs/sites/default/modules/commerce/modules/checkout/theme/includes/commerce_checkout.pages.inc' for inclusion (include_path='.:') in theme() (line 794 of /httpdocs/includes/theme.inc).
Warning: include_once/sites/default/modules/commerce/modules/checkout/theme/includes/commerce_checkout.pages.inc) [function.include-once]: failed to open stream: No such file or directory in _theme_process_registry() (line 396 of /var/www/vhosts/koziol.co.il/httpdocs/includes/theme.inc).
Warning: include_once() [function.include]: Failed opening httpdocs/sites/default/modules/commerce/modules/checkout/theme/includes/commerce_checkout.pages.inc' for inclusion (include_path='.:') in _theme_process_registry() (line 396 of/httpdocs/includes/theme.inc).

I removed the first part of the file paths in the warnings above.

The review pane does appear, but I have the order total for the products lets say $100, and then I have the total for the shipping $20, but there is now place with the sum of both - $120 .

Thanks

Idan

In order to display the total for both products and shipping, I modified the view filters, but the problem is that products use the product name field, and this results in - unknown shipping method for shipping, and if I use the label field, then I get the sku for the product.

How did you resolve this?

Idan

I think many of the shipping calculation and also some coupon calculation problems arises because we try to mix up two things:

1. Normally you have some products in your shopping cart. Each of this item has at least a quantity, a name and a unit price.
2. The other thing you have is so called order totals. Order total rows differs in that way you have only a price and a name. You don't want to show up additional information such as the product attributes or the unit price (only the total).

At the moment you can add the second sort of things only by adding a "price component", but this price component bases again on a item line. That means you need to assign a price component at least to one line item. But for shipping you don't want to show up a quantity or something similar to the customer. So this could not work at all.

Most of the shopping carts (Magento, osCommerce, xtCommerce...) distinguish between these two things. (By the way PrestaShop doesn't and they are struggled with things like "payment fees" and other trivial things...) I think we should consider to add such a feature to the core of commerce, because there are many use cases for that:

• Coupon System (fixed amount)
• Shipping Fees
• Fixed Taxes
• Payment Fees

Another closely related problem is that we need an option to show up the tax rate per line item in a separate column. So we need a option to split up the price component into single columns. For example we need a formatter with the option to say which component should be displayed.

I have build a prototype module to show how I propose the implementation of the order total display. The module can be found in the attachment. I also add a screenshot.

Implementation

I add an area field. Its very similar to the existing order total area field. I replace it with a view handler. So instead of displaying the hard coded price formatter output, a view output is presented.

To build the view I add a "order total line type". This type extends the item line type. In the definition of the line item type you can add it to the "order total line type" list. Each total line type has a weight (for sorting) and it can be optionally activated or not.

Before the view is rendered a calculation callback is executed, so each type can add / update order lines.

The benefits of this integration:

• Fixed amounts (coupons, shipping amounts etc.) can be defined by modules.
• Better control over styling the order control table.
• More intuitive way of handling non-product amounts.
• The user has better control over the elements displayed in the order total table

Known Issues

• Calculation is wrong
• Shipping module is not integrated.
• No Backend UI (For defining the weights)
• Order total weights are not respected.
• Tax module is not integrated.

At the moment I am working on the coupon module. There we need this or a very similar functionality to handle fixed amount coupons. Therefore I propose to add this module or a similar module to the core of commerce. Its also possible to merge with the commerce_line_item module. But I think a separate module is better, because we want perhaps also non order total line items.

Are there any interests of the core development team to add the proposed functionality to the core of commerce?

I'm uneasy about this approach, as the existing area handler interacts a lot more with price components. These aren't accessible to Views at all since they're a part of the field's data array - I'd love to break these components out in 2.x if possible. While this is a nice way to display aggregate line item totals, it won't work for something like taxes which solely works through price components.

I think instead what might make more sense is to evaluate how to better make use of the components array for the order total. For example, product line items get this "base_price" component on each line item total that is added up to make the "Subtotal" line in the order total display. For different types of line items, you could use a different component name ("coupon_3", "shipping_ups_ground", etc.) so they appear on their own in the order total display. Additionally, there's an alter hook for price components that can be used to reorder and adjust the display of these components as necessary.

I don't have time at the moment to elaborate further - does this give you enough to work with?

Hi Ryan

Thanks for reviewing my approach. I have to say two things:

1. It is right, that this approach is not capable to calculate percentage amounts. But my approach is not a replacement for the existing component approach, it is a enhancement to it. It is easy to build a wrapper around the tax to turn the tax amount into a order line.
2. The major problem is, that I need for each coupon a own component. If we had 10'000 coupons, we get a problem, if we try to handle with the component hook. (You can't hold all these in the memory.) The other problem I have is the impossibility to handle non percentage amounts.

I hope you can explain more detailed how you would like to go ahead with the described problems, if not I have to set this module as a dependency for the coupon module.

You're right, googletorp. I didn't realize which issue this had been posted in. : )

A separate issue would be better.

To bring this issue back around, let's try rebooting comments at #7. I won't commit this for beta3, but if need be we can address it in person with Bojhan at the sprint in June.

From a theming perspective - the whole checkout process is a tad off.

On the cart - we have the option of a view that can be a table/grid/list etc.
On the checkout form - same again, with the summary block.
Review - all table based, except pulls the summary block in.

From a css pov - gets a bit tricky to target similar items across these pretty similar pages, so I think in the end more control across them all would be great for themers!

subscribe

Also please stop testing this solely with Bartik. The most common themes with Drupal are Zen, Fusion or Acquia Marina. If you use common drupal css's and id's the forms should look decent on all themes. I think it's kind of important to look good out of the box.

If this included adding some styling which doesn't look great, but can be copied and moved into your theme, I suggest we go for that. This will reduce everyone having to create unique styles for each site they work on and instead they'll just need to modify some colors.

Additionally I've noticed a discrepancy of the styling between the "checkout" and "checkout review" displays. This causes extra work when you move panes between these displays. Each common pane (no matter what display) should contain a common class, and this is where styles should be applied. An additional class needs to be added to it's direct parent or direct child which should identify it as a "checkout" or "checkout preview" pane. You can not put two styles on the same element, as IE6 doesn't allow for this.

Examples

<div class="pane-type-checkout"><div class="pane-billing-information">
...
</div></div>

I've also noticed that occasionally when you submit a "checkout" with errors, some times they classes change. This also causes problems, so please do not depend on the auto-generated styles as the occasionally disappear Anyone else encountered this? And if so, when exactly does it happen and how can we avoid it?

+1 for divs over tables. I'll always vote to err on the side of more theming control over looking nice in a "default" theme.

Also, it would be great to have specific ids or classes on each of these elements as well. In my case I'd like to be able to hide the username (in the Account information section).

+1 for @googletorp's patch. Tables are not the right markup for presenting the review page. Semantic hierarchy is provided by divs and headings -- that's completely appropriate. Visual hierarchy is strictly up to the theme. If there's not enough visual hierarchy in Bartik's H3's, that's an issue for the Bartik queue.

I also agree that some classes for the panes are appropriate; I'll try to throw a patch together.

I have found that this can be done with a template file that is named appropriately. I made this template file using some of the logic from the proposed patch and found that it works in Zen, Omega, Fusion, Bartik without creating a preprocess function. This would make an easy way for a themer to over-ride the table in the review pane.

Name it commerce-checkout-review.tpl.php and fill it with :

<div class="review-panes clearfix">
  <?php foreach ($variables['form']['#data'] as $pane_id => $data):?>
    <div class="review-pane <?php print $pane_id;?>">
      <h3 class="pane-title"><?php print $data['title'];?></h3>
      <?php print $data['data'];?>
    </div>
  <?php endforeach;?>
</div>

Attached is a reroll of googletorps patch with a couple small changes. It uses H2's instead of H1's and adds a class to the wrapping div so it's not just an empty clearfix div. I still feel pretty strongly that tables are the wrong semantic elements for this page.

@ryan's point, I understand your point about how the table view can be considered easier for a user to consume the data. I think first of all that it's subjective -- most themes we make, the review panes will look much better with this semantically relevant markup compared to the hard coded styles in the current implementation. Even if that weren't so (and it's also subjective) -- you can achieve the exact same appearance with semantic markup if that's a hard requirement.

Screenie from bartik attached for reference.

Wow, the screenshot looks great. Thanks for the iteration here - I'll try to review ASAP so we can make sure it's in 1.3.

We didn't need 20 comments and a year, just a bump in my tracker with yet another screenshot showing it looking decent. ; )

Oops thanks googletorp .. that was sloppy of me.

googletorp: The patch was wonderful! Thank you!

Let me know if this is a completely different issue but Would it be possible to change the weights of the items on the review page? I know they follow they inherit the weights the checkout page, but this isn't necessarily the order everyone wants them.. Anyone else experiencing this?

Tagging for 1.3.

There was some issue with the preprocessor not getting picked up by the theme registry since it was in a file not referenced by hook_theme. I went ahead and moved it over to commerce_checkout.module, as well as renamed it to "template_preprocess_commerce_checkout_review".

Can someone please review if this was the result everyone wanted? I'm basing the patch off of #28, since applying #32 directly was missing a bit of vital functionality.

Would be a good idea if I attached a patch :P

I wonder if in conjunction with this patch we should change the default "fieldset or no fieldset" option for the review pane. Best to still leave it off?

Have we made a decision/indecision about this yet? Maybe if you get the chance can you review the patch?

Well, after 13.5 months, I suppose we should award googletorp his "Patience Achievement." Congratulations! I think it's worth about 10,000 XBOX points or something. : P

Thanks to everyone who helped drive this home. I hope it improves the theming experience and doesn't break every existing theme. :-/

Ok, so after committing this, I started to experience a case of buyer's remorse, and in an e-mail chain, Damien kind of confirmed it. The issue with this patch is that even though it provides a better theming experience for new sites and themes moving forward, it potentially breaks the checkout themes for every existing site. This means that in order to upgrade to Drupal Commerce 1.3, everyone would have to go address the change to the themable components on the checkout review pane, especially since we're changing the container element from tables, trs, and tds to divs - it's quite likely that people introduced explicit table styling that would suddenly be deprecated with no potential intermediate solution available.

A similar issue has arisen in #1030128: Convert shopping cart block and form Views to use Views block / page displays where I thought we may be able to provide an upgrade path to the default cart Views but finally decided there is no safe way to do it.

What I'm going to propose is basically a parallel path, where we revert this patch and instead move the code to a contributed project that encapsulates this change in a standalone module. A second module in that project would also apply the changes to the cart Views that that linked issue made. Basically, we'd be pre-backporting UX / theme improvements from the non-existent 2.x branch - it's very sci-fi of us.

Thoughts? Can this possibly go down any other way? (The only other thing I can come up with is supporting both in core, but that just may not be necessary or advisable.)

Yeah, committing it earlier would've been better. : (

The two options as I see it are to preserve the original HTML with an option in the checkout pane to use this or to place it in another module that people can simply turn on to get the new HTML.

FWIW: I think projects like commerce are subject to this refining and those working on/with the project understand that. Whether the patch is committed in the beginning or late in the game, the point is that it is still committed, and really makes the designing process a lot easier, IMO.

With commerce growing everyday, and with the amount of modules being released, I personally think it is a good idea to accept the new markup as standard and let people know of the changes. I am against the table approach, while it looks good out of the box, it is hell to design a pretty checkout review page with it, also in my opinion.

I don't know if we should go by what was in the past just because people have that established. My understanding as a noob, with all updates, it is the duty of the updater to make sure that things work with the site, where as it is the duty maintainer to make sure the updater knows about it.

FWIW again, I'm not exactly the most fond of making a separate module also. To me that seems like using duct tape to fix a leak.

These are merely my opinions.

Just a note to this, if not mentioned already, the review page is of course themable

See

/modules/checkout/includes/commerce_checkout.pages.inc 
line 380
function theme_commerce_checkout_review($variables) {}

for the default implementation, copy to template.php to override.

cheers,

DT

Update, I see this is referenced in the patches, but might be useful for someone wanting to change the review page in the current version.

Oooh, I like that, too. I wonder why I never opted for that instead of going for the table.

Sigh, just proving my lack of front end credentials. :-/

I think a good solution is to have an extra option in "admin/commerce/config/checkout/form/pane/checkout_review/".
Btw, for new drupals, it must be the default option, tables demands lot of time to theme and it's annoying.

is there any chance to apply the same .tpl to Checkout:Checkout??
Titles in Checkout:Checkout only appears if you display pane as fieldset. Also, titles are in legend tag and there are no .
Plus, having a template it's always better to layout or add extra texts.

greetings

Here's a patch to undo what has been done.

Alrighty, I've committed that - funny patch name, btw. ; )

Let's go ahead and leave this active as a feature request; I'm wondering what to name such a module holding this and whether or not it should include other potential review pane styles such as the fieldset approach posted above.

Correct me if I am wrong: Does 1.3 no longer allow for #48 to function? It appears that there is no longer a .tpl in that location.

UPDATE: Edited template.php and added this

function THEME_NAME_theme(){
  $hook['commerce_checkout_form_review'] = array(
    'path' => 'templates/preprocess/commerce_checkout_review',
    'render element' => 'form',
  );
  return $hooks;
}

function THEME_NAME_preprocess_commerce_checkout_review(&$variables) {
  $panes = array();
  foreach ($variables['form']['#data'] as $pane_id => $data) {
    $panes[$pane_id] = array(
      'title' => $data['title'],
      'data' => $data['data'],
    );
  }
  $variables['panes'] = $panes;
}

Then used #48 tpl. All is well.

The release notes for 1.3 are slightly misleading. For anyone else scratching their head, this change never actually made it into 1.3.

Ii was not realesed since it will break all existings commerce checkout theming. If you need this theming, re/apply the patch #40.

It would be great to have an extra clean up at commerce_price.module ->

function theme_commerce_price_formatted_components($variables) {
  // Add the CSS styling to the table.
  drupal_add_css(drupal_get_path('module', 'commerce_price') . '/theme/commerce_price.theme.css');

  // Build table rows out of the components.
  $rows = array();

  foreach ($variables['components'] as $name => $component) {
    $rows[] = array(
      'data' => array(
        array(
          'data' => $component['title'],
          'class' => array('component-title'),
        ),
        array(
          'data' => $component['formatted_price'],
          'class' => array('component-total'),
        ),
      ),
      'class' => array(drupal_html_class('component-type-' . $name)),
    );
  }

  return theme('table', array('rows' => $rows, 'attributes' => array('class' => array('commerce-price-formatted-components'))));
}

Overriding "table>tbody>tr>td" to a simple "ul>li>span" theming. It would be far better to apply CSS.

Many thanks for all your efferts about this issue.
Greetings.

i got a fatal error after using code from #54. after removing the non necessary hook_theme() #48 works.

I think the main problem with the checkout review is that the table tr's does not have any class or id identifier and are not themable by css or js.
(please just add identifiers to the table rows in the next commerce update)

Benchi, tables are problems. it's much better to css with divs and not with table. div = "div". table = "table>tbody>tr>td"
If you need to CSS a tag with no class, use pseudo CSS. Like "td:first-child" or "td:first-child+td" or "td+td". You can always select tags without class. Check this tutorial for all selectors list.

Yes but we could improve the markup a lot just by adding classes. this would could happen right now and dont need to be in a 2.0 release.

Wish I had a "Like" button for comment #62. ; )

I'm a bit late on this issue but I think the problem should be "reversed"...
- re-apply the patch in #40
- provide a contributed module or simply an alternative template file to be copy/pasted so that existing sites can quickly have the old markup

This way drupal_commerce can provide good and sensible default.

That would be fine if my comment in 43 were misguided, but it's simply insufficient to change something in core in a stable branch and then ask everyone who didn't want it to change to go download a contrib module. We'll be opening the 2.x branch soon enough and can fix it, though, and in the meantime a contrib could be made available that fixes it for those who want it. We could even include such a module in Commerce Kickstart, since it would be used for new sites and have no chance of interfering with existing themes.

#63 worked great.

Unrelated to the switch from tables to DIVs, I have a small backwards compatibility concern ... all the patches and theme advances here are assuming each pane's 'data' element is a pre-rendered HTML field (suitable to drop into '#children' or otherwise directly output).

This represents a change to the pane behaviour which has historically (up to and including the most recent stable and dev releases) catered for an array of key-value pairs.

    // Next, add the data for this particular section.
    if (is_array($data['data'])) {
      // If it's an array, treat each key / value pair as a 2 column row.
      foreach ($data['data'] as $key => $value) {
        $rows[] = array(
          'data' => array(
            array('data' => $key .':', 'class' => array('pane-data-key')),
            array('data' => $value, 'class' => array('pane-data-value')),
          ),
          'class' => array('pane-data', 'even'),
        );
      }
    }
    else {
      // Otherwise treat it as a block of text in its own row.
      $rows[] = array(
        'data' => array(
          array('data' => $data['data'], 'colspan' => 2, 'class' => array('pane-data-full')),
        ),
        'class' => array('pane-data', 'even'),
      );
    }

I realise it's a while since any core Commerce panes used this feature, but I'd still be against removing it. (TBH I'd like to see Commerce make use of it again, it seems like something that should be handled at the point of theming the review page instead of earlier.)

It should be easy enough to incorporate it into the proposed commerce-checkout-review.tpl.php, surely? Something like this:

foreach ($panes as $pane_id => $pane) {
  if (is_array($pane['data'])) {
    $output = '';
    foreach ($pane['data'] as $key => $value) {
      $output .= '<div class="pane-data-key">' . $key .'</div>';
      $output .= '<div class="pane-data-value">'. $value .'</div>';
    }
  }
  else {
    $output = $pane['data'];
  }
  echo theme('fieldset', array('element' => array(
    '#title' => $pane['title'],
    '#children' => $output,
    '#collapsible' => false,
    '#attributes' => array('class' => array('pane', $pane_id)),
  )));
}

I'd even go so far as to suggest a theme_commerce_checkout_review_pane_data(array('pane' => $pane)) kind of function, but maybe that's a bit much.

Unless of course this was officially removed a while ago and this is just legacy code, I don't know. I'm just looking at the code in the theme function.

52: ruin-markup-1098028-52.patch queued for re-testing.

See comment #43. Let's leave this active.

I really Agree with having the checkout review in forms of divs. Any progress so far on this?

No, because the Drupal philosophy is to develop something until it's about 60% finished, then release it and start working on the next version and refuse patches to the current version with the idea that they should be backported which never happens.

This is the Drupal way:

https://www.drupal.org/node/1559116 (2 years, 6 months and still not fixed)
https://www.drupal.org/node/242048 (2 years, 9 months between the issue being raised and the eradication of the Blue Smurfs with the release of D7)
And then there's this issue with is 3 years, 7 months old.

You can, however, override the output with theme_commerce_checkout_review()
Put the whole thing in your template.php and replace the theme_ prefix with yourtheme_

http://drupalcontrib.org/api/drupal/contributions!commerce!modules!check...

#63 works great for me

#63 worked perfectly - thanks!

This issue is open since a long period of time and the current table solution is truely horrible and bad for styling individualisations. Sadly I have to agree with #76 more often, which is truely sad (not criticising and especially NOT rszrama who is one of the most active and fastest developers I know here).

But let's get back to the core points. The discussion is much too long and finally there is no solution yet. It's still tables in the latest .dev release and I can't see any of the solutions from above in code.
Please let's find a solid working solution.

So here is finally what we need:

• Remove the table and replace it by semantically proper markup (fieldsets are OK becuase of consistency I think even if there are no fields in this case)
• Add classes to make the different elements selectable in CSS / JS

All this is solved in #63.

Anyway there are two improper results from the current status:

1.

In #63 the $pane_id should be incapsulated in a drupal_html_class() to have a clean class name (that's easy, here's the full code for the commerce-checkout-review.tpl.php):

foreach ($panes as $pane_id => $pane) {
  echo theme('fieldset', array('element' => array(
      '#title' => $pane['title'],
      '#children' => $pane['data'],
      '#collapsible' => false,
      '#attributes' => array('class' => array('pane', drupal_html_class($pane_id))),
  )));
}

2.

I do not agree that it's a good idea to keep this bad table markup because old page styles could break and provide no final solution from this important issue.
We should really discuss the following options and come to a result:

1. Change the table markup to a proper markup by a patch from above in the module directly and document it in an upgrade message to that webmasters can fix the styling
2. Create a little helper module to simply override the bad old markup easily
3. Add a documented "_better_markup_commerce-checkout-review.tpl.php" or something like that in the commerce_checkout submodule to be used by themers to override the default implementation
4. Or the other way around implement the better solution from (1.) by default and provide an override to activate for old pages to keep the old ugly markup.

These are my ideas, perhaps you will have other suggestions. The current markup is no acceptable solution for modern (responsive) websites and developers should not be required to read this very very loooooooooong issue like we did...
I'd especially hope for szrama's opinion, because he's the one who should make the final decision about it.

+1 for #80 > 2.4 sounds very good.

I'm not sure why I didn't propose it before (or maybe I did up above somewhere, haven't read it) - we could always ship with both. We could use a variable in the checkout pane's settings form to use the old table style markup, setting it in an update function to preserve that behavior for existing sites. However, new sites would get the div style markup committed and reverted way back when to have a better form.

Wow that would be great news, I'd love that! :)

Alrighty, this code is going to make it into the new Commerce Responsive UI module. Huzzah!

https://www.drupal.org/project/commerce_responsive_ui

I'm going to close this out in this queue now, and I'll make sure we don't end up with similarly awful markup in Commerce 2.x.

Whao, thank you very much, @rszrama! :)

Could you create a dev release for testing, as soon as it makes sense? [Of course lets not further discuss that here, it's a separate issue, I know] ;)

You bet! In fact, I'll go ahead and open an issue there specifically to track migration of this patch into that module. Josh Miller is the lead on it for now w/ me just providing moral support and encouragement. ; )