[DONE] Shopping Cart bug

Unfortunately the bug fix for tpl_shopping_cart implemented in 1.3.0.2 has created a new problem.

A div is always created at line 13 with class sideBoxContent. However, if the shopping cart is empty, a second div with the same class is created at line 42 nested inside the first one.

This is a problem because if the class is used to style sideboxes e.g. with image-based borders, then the image is duplicated (to the extent that spqace permits) inside the shopping cart sidebox.

The fix is easy. Replace line 42> $content .= '<div class="sideBoxContent center bold">' . BOX_SHOPPING_CART_EMPTY . '</div>';with > $content .= '<span class="cartEmpty">' . BOX_SHOPPING_CART_EMPTY . '</span>';eliminating some inline CSS at the time.

Here's the file with my recommended solution >> Attachment #260

Thanks for the report, Kuroi.

There is a class-consistency problem there.

NOTE: tpl_shopping_cart.php is the ONLY sidebox which uses the "sideBoxContent" class more than once.... and in fact, it uses it 4 times: outer (like all the rest), plus "empty cart", "send GV", and "GV balance".

Line 13 is the overall container for the sidebox content. This is consistent with all the other sideboxes:```php
$content .= '<div id="' . str_replace('_', '-', $box_id . 'Content') . '" class="sideBoxContent">';

Line 15 is for info IF cart is not empty: ```php
  $content .= '<div id="cartBoxListWrapper">' . "\n" . '<ul>' . "\n";

line 42 is for info if cart IS empty:```php
$content .= '<div class="sideBoxContent center bold">' . BOX_SHOPPING_CART_EMPTY . '</div>';

Lines 58-59 are for other info:```php
      $content .= '<div id="cartBoxGVButton"  class="sideBoxContent"><a href="' . zen_href_link(FILENAME_GV_SEND, '', 'SSL') . '">' . zen_image_button(BUTTON_IMAGE_SEND_A_GIFT_CERT , BUTTON_SEND_A_GIFT_CERT_ALT) . '</a></div>';
      $content .= '<div class="sideBoxContent center bold">' . VOUCHER_BALANCE . $currencies->format($gv_result->fields['amount']) . '</div>';
```I suppose what one does with this one depends on how you want content to appear; however, I suspect calling it cartBoxContent may be logical


Lines 42, 58 and 59 should not share the same class from the outer container (ie: line 13).

Suggestion: 
Line 15 remains id=cartBoxListWrapper
Lines 42, 58, 59 changes references to sideBoxContent class to a NEW class: cartBoxContent, which could simply be an extension of some existing selectors:
> #cartBoxListWrapper li, #ezPageBoxList li, .cartBoxTotal**, .cartBoxContent **{
> 	margin: 0;
> 	padding: 0.2em 0em;
> 	}

I have a strong feeling of déjà vu. I propose a solution for a problem in this template and you come up with a better one that fixes a wider issue. Haven't we been here before :yes:?

Some minor comments:

1. applying the class cartBoxContents to line 42 would leave no option for treating the empty box text differently to the voucher text, even though they are clearly different things, though the converse is not true because of the cartBoxGVButton id
2. cartBoxContent seems like a very generic name that, if met in a stylesheet, I would personally expect to apply more widely than is being suggested here, in particular I would expect it to also apply to the content that is actually contained cartBoxListWrapper
3. Although I correct my earlier suggestion that the center and bold are inline CSS, they are of course classes, they have the same effect, i.e. prescriptively assigning presentational information to these elements
   My personal preference would therefore be:1. line 42 - give it its own id (not class) e.g. cartEmpty and drop the classes altogether
4. line 58 - drop the class, as it now applies only to two different things (i.e. on this line a link attached to a button, and on the next line plain text) and so does not really add any value.
5. line 59 - change the only instance of this class into an id and rename semantically e.g. cartVoucherBalanceHowever these are but minor refinements to your suggested solution.

Your counter-proposals are definitely a better refinement.

Perhaps you can clarify for me your recommendations on the following, as I would like to touch as few files as necessary, but certainly not be short-sighted:

• line 42 -- keep or drop the secondary classes (since an empty cart much less likely to present a case where specific styling would need to be used to override a logical presentation of "center bold"):```

• line 58 -- just the ID:```

• line 59 -- keep or drop the secondary classes:```

Retaining the secondary classes means there is no need to add the new ID's to the default stylesheet. Dropping them means adding to stylesheet.

kuroi:

I have a strong feeling of déjà vu. I propose a solution for a problem in this template and you come up with a better one that fixes a wider issue. Haven't we been here before :yes:?
LOL -- that's where teamwork creates a greater synergy.... working towards the "best" solution, not just a bandage ;)

DrByte:

Perhaps you can clarify for me your recommendations on the following, as I would like to touch as few files as necessary, but certainly not be short-sighted:

• line 42 -- keep or drop the secondary classes (since an empty cart much less likely to present a case where specific styling would need to be used to override a logical presentation of "center bold")

• line 58 -- just the ID:

• line 59 -- keep or drop the secondary classes

Retaining the secondary classes means there is no need to add the new ID's to the default stylesheet. Dropping them means adding to stylesheet.Embedding this sort of information via inline CSS or classes removes flexibility for the site designer. However, in the context of minimising changes for a maintenance release, and the lack of problems that this is causing anybody (otherwise the bug would have been spotted sooner), and given the huge improvements that the rest of changes will make, this approach, as listed by you above, seems completely justifiable here.