Patch needs review: Updates to the shopping cart block

16 replies [Last post]

Fri, 01/30/2009 - 10:32

Ryan

Offline

Joined: 08/07/2007

Juice: 15438

Two long standing issues in core Ubercart have been our excessive use of cache_clear_all() and the awkwardness of the shopping cart block for themers. I dug into the first issue last night and ended up addressing the second at the same time, though this does mean that themes currently dealing with the shopping cart block will be broken after our next release. This work is for Ubercart 2.x.

A quick summary of the changes in this first patch includes:

If caching is enabled and a user is not logged in, the shopping cart block title will always use the empty cart icon and won't have collapsibility. The contents will come from a new theme function, theme_uc_cart_block_content_cachable(), which at this point simply includes a short message and link to the cart page. This should get rid of those "ghost" carts the people get when they have page caching enabled.
Authenticated users will see the cart block as before... so, not really a change, but just wanted to make sure no one freaked out.
All the code has been removed from the cart block theme functions. The appropriate bits of information are now passed in as arguments for theme functions to use as they please.
The HTML and CSS used has been updated to make a little more sense... no more specifying element names in class names. How weird was that? Did I really know what I was doing when I first wrote those??
A total list of the theme functions now includes:
theme_uc_cart_block_title() - themes the block title
theme_uc_cart_block_title_icon() - themes the cart icon in the block title
theme_uc_cart_block_content_cachable() - themes the cachable cart block contents... should not be used for any dynamic text!
theme_uc_cart_block_content() - themes the normal block contents
theme_uc_cart_block_items() - themes the collapsible list of items in the cart
theme_uc_cart_block_summary() - themes the summary of items in the cart with the total and view/checkout links
Removes generic uses of cache_clear_all() from core that were being used inappropriately to try and keep anonymous carts from caching "ghost" items.

Please test and review this patch, especially if you intend to be theming these components!

Please note: The original patch has been replaced with the latest patch from below, currently comment #2.

Preview	Attachment	Size
	uc_cart_block_rewrite.patch	25.12 KB

Fri, 01/30/2009 - 11:29

#1 View page

greggles

Offline

Joined: 12/12/2007

Juice: 95

image paths

For the image paths, you could consider using file_create_url($name_of_file) or url($name_of_file)

<?php
// Use the "empty" cart icon.
            $cart_image = $uc_cart_path .'/images/cart_empty.gif';
            $cart_image = file_create_url($uc_cart_path .'/images/cart_empty.gif');
            $cart_image = url($uc_cart_path .'/images/cart_empty.gif');
?>

Should be slightly more reliable for subfolder situations and is more "Drupaly."

Similarly, instead of using the concatenation to build up images something like theme_image might be better.

I'm not really familiar with the problems in the past for these blocks, so I'm just offering general "make more Drupaly" advice - perhaps some of these would be seen as regressions in terms of how themers want to override things.

Greggles Drupal Articles

http://knaddison.com | http://growvs.com | http://certifiedtorock.com/u/36762

Fri, 01/30/2009 - 12:00

#2 View page

Ryan

Offline

Joined: 08/07/2007

Juice: 15438

Re: image paths

Thanks for weighing in, Greg! I think I'll take your advice to remove base_path() from the hook function and use theme_image() in the appropriate theme functions for building the img HTML. The attached patch is a re-roll of the first that does this along w/ removing the excess uses of cache_clear_all() from a few modules.

Attachment	Size
uc_cart_block_rewrite.patch	25.12 KB

Fri, 01/30/2009 - 15:01

#3 View page

stephthegeek

Offline

Joined: 10/20/2007

Juice: 575

Re: Re: image paths

I just checked out what's on the livetest... why isn't the block showing what's in the cart? It seems you have to click on the "View" link whether it's empty or has stuff in it.

Gorgeous original Drupal themes (and Ubercart themes!) ~ Psst: more Ubercart themes on our new site

Fri, 01/30/2009 - 15:05

#4 View page

Ryan

Offline

Joined: 08/07/2007

Juice: 15438

Re: Re: Re: image paths

Yeah, for anonymous users when caching is turned on, that's how it will work. Otherwise what's been happening is an anonymous user will come, put something in their cart, and then visit a site on the page that hasn't been cached. That page then becomes cached w/ a cart block that looks like it has stuff in it. To get around that, if page caching is turned on, the block becomes this little static affair. It can be themed, though.

If I turn caching off on the Livetest (will do that right now) you'll see the good ol' block.

Fri, 01/30/2009 - 15:08

#5 View page

Ryan

Offline

Joined: 08/07/2007

Juice: 15438

Re: Re: Re: Re: image paths

There is a contribution that will load the cart block contents through AJAX to get around this caching issue I believe.

Fri, 01/30/2009 - 17:56

#6 View page

Ryan

Offline

Joined: 08/07/2007

Juice: 15438

Re: Re: Re: Re: Re: image paths

I've committed what's here to Bzr, but it's still up for review if anyone sees any problems or mistakes here.

Changes in this patch are I used the constant CACHE_DISABLED instead of 0 to determine if caching is on, and I also included in here (kind of on accident) some changes to the checkout messages and their formats based on this issue.

Attachment	Size
uc_cart_block_rewrite.patch	32.73 KB

Fri, 01/30/2009 - 22:42

#7 View page

stephthegeek

Offline

Joined: 10/20/2007

Juice: 575

Re: Patch needs review: Updates to the shopping cart block

Hmm.. ok, so at least it's themable now But we could do a lot better to make most things editable in CSS, to be able to drastically alter the look without needing to override the entire chunk.

Here's what I'm thinking, from top to bottom (just throwing it out there for now, still haven't rustled up any actual coding time...)

- retool how block title icons are done -- icons need to be specified in CSS, not img tags
-- use text-indent? will need to test this, not sure what's the best approach with the collapsible block
- add meaningful alt properties to icons
- add even/odd classes to tr.cart-block-item rows
- add span and .num-items class to number in .cart-block-summary-items
- remove strong tag on Total:
- add label tag to Total:
- move .cart-block-summary-checkout class up from td to tr
- remove parentheses on (View cart) (Checkout)
- turn summary links into inline unordered list, with .first and .last classes
- add unique classes to links -- cart-block-view-cart, cart-block-checkout
- add border-right on checkout list items, remove on .last so you end up with: View cart | Checkout

Are there any other common modules that add to or alter things in this block that we should be aware of?

Gorgeous original Drupal themes (and Ubercart themes!) ~ Psst: more Ubercart themes on our new site

Fri, 02/06/2009 - 12:34

#8 View page

Ryan

Offline

Joined: 08/07/2007

Juice: 15438

Re: Re: Patch needs review: Updates to the shopping cart block

So, I'm gonna need some help to get the icon stuff done.

stephthegeek@drupal.org wrote:

~~- add even/odd classes to tr.cart-block-item rows~~
~~- add span and .num-items class to number in .cart-block-summary-items~~
~~- remove strong tag on Total:~~ (there was no more strong tag; I'm using font-weight: bold; in uc_cart_block.css)
~~- add label tag to Total:~~
~~- move .cart-block-summary-checkout class up from td to tr~~ (I'm assuming you meant .cart-block-summary-links)
~~- remove parentheses on (View cart) (Checkout)~~
~~- turn summary links into inline unordered list, with .first and .last classes~~ (did this through theme_links())
~~- add unique classes to links -- cart-block-view-cart, cart-block-checkout~~
~~- add border-right on checkout list items, remove on .last so you end up with: View cart | Checkout~~ (please see the 2.x Livetest... there's an extra funky bit of space in there that I can't find and eliminate)

These have all been taken care of in the attached patch. I made the description rows the same odd/even class as their associated product, so they're not true odd/even rows... I think conceptually this is the way to go, but let me know if you disagree. As to your last question, the cart block hasn't been extensible, so nothing else can really make it in there for now.

I also setup a "sub-tracker" for theme layer improvements using the theme layer tag on d.o.

Attachment	Size
cart_block_updates.patch	6.47 KB

Fri, 02/06/2009 - 17:19

#9 View page

stephthegeek

Offline

Joined: 10/20/2007

Juice: 575

Woohoo! Yes, that makes

Woohoo!

Yes, that makes sense for even/odd.

So, for the icons, here's what I'm thinking for markup and CSS. I worked off of the current code on the livetest. I tried to use similar classes but the markup will probably need adjustment for actually implementing the show/hide block stuff.

For the markup, replace everything inside the h2 block title with what's in here:

  <h2>
    <a href="/uc2/cart" class="cart-block-title-bar cart-block-toggle ucCollapseBlock-processed" alt="Show/hide shopping cart contents" title="Show/hide shopping cart contents"><span class="cart-block-title-text">Shopping cart</span><span class="block-cart-arrow arrow-up"></span></a>
  </h2>

CSS:

.cart-block-title-text {
  display: inline-block;
}
.block-cart-arrow {
  display: inline-block;
  padding: 2px 15px 8px 15px;
  height: 7px;
}
.cart-block-title-bar .arrow-up {
  background: transparent url(images/bullet-arrow-up.gif) no-repeat center center;
}
.cart-block-title-bar .arrow-down {
  background: transparent url(images/bullet-arrow-down.gif) no-repeat center center;
}
.cart-block-title-bar {
  background: transparent url(images/cart_full.gif) no-repeat left center;
  padding-left: 22px;
  text-decoration: none;
}
.cart-block-title-bar:hover {
  text-decoration: none;
}

This puts the icons inside the link tag, with the cart icon as a background on the left, and a span for the arrow. This drastically reduces the markup and allows for much more theming through just CSS.

I worked on the assumption of an .arrow-up/.arrow-down class being added on the span through jQuery, feel free to shuffle classes/IDs around as needed. The inline-block on the arrow to get the positioning similar across browsers is a bit tricky so any tinkering with those dimensions will need to be done carefully, but it should be solid. The previous CSS for the arrow/cart icon can be removed.

Tested in FF2/3, IE6/7, Safari 3, Opera 9.6.

Gorgeous original Drupal themes (and Ubercart themes!) ~ Psst: more Ubercart themes on our new site

Fri, 02/06/2009 - 17:39

#10 View page

Ryan

Offline

Joined: 08/07/2007

Juice: 15438

Re: Woohoo! Yes, that makes

Thanks, will implement this ASAP. Any thoughts on the phantom padding between the right border on the "View cart" link and left boundary of the "Checkout" link's li element?

Tue, 02/10/2009 - 17:39

#11 View page

Ryan

Offline

Joined: 08/07/2007

Juice: 15438

Re: Woohoo! Yes, that makes

Hey Steph, was going through and implementing this when I realized that it changes the behavior of the title bar so that the whole thing is a link to the cart instead of retaining the concept of the title and icon being used for collapsing/expanding the block. Previously, just the icon was used as a link, but now with the icon simply being a background image, will this even be possible? Or should I be figuring out how to select the cart block title and icon separately... maybe returning false when those links are clicked and doing the collapse/expand instead.

I noticed, too, that your code doesn't accommodate changing the icon based on empty vs. full. Was that intentional, or should I add a class to specify which version of the icon to use?

Wed, 02/11/2009 - 03:20

#12 View page

stephthegeek

Offline

Joined: 10/20/2007

Juice: 575

Ryan wrote:Hey Steph, was

Ryan wrote:

I noticed, too, that your code doesn't accommodate changing the icon based on empty vs. full. Was that intentional, or should I add a class to specify which version of the icon to use?

Hmm I was working off of the existing markup -- the title was already a link for expanding/contracting which linked to /cart, I assume if JS is off? I didn't re-do the JS magic for that though. It should still work just fine to do the collapse with it.

Ah that wasn't intentional with the icon -- if you add another class to it that would be fine.

Gorgeous original Drupal themes (and Ubercart themes!) ~ Psst: more Ubercart themes on our new site

Tue, 02/17/2009 - 17:20

#13 View page

Ryan

Offline

Joined: 08/07/2007

Juice: 15438

Re: Ryan wrote:Hey Steph, was

Ok, one more patch... incorporating Steph's suggestions and then some. I cleaned up the JS, reduced the need for some classes, cleaned up the CSS, removed the need for a couple variables in the theme functions. The images are now being handled entirely by CSS, and there are appropriate alt/title tags on everything. Woohoo! It could use some testing, but I'm quite pleased with the results. Patch attached!

Attachment	Size
cart_block_css_images.patch	10.58 KB

Wed, 02/18/2009 - 01:16

#14 View page

Ryan

Offline

Joined: 08/07/2007

Juice: 15438

Re: Re: Ryan wrote:Hey Steph, was

Mike O'Connor mentioned it had quite a bit of spans in the title, so I tested with taking the span off of the title text. Everything functions the same... I'm just curious if there's any particular reason to keep the span around the title text as in the patch above or if what is in place now on the 2.x Livetest will suffice.

Wed, 02/18/2009 - 03:00

#15 View page

stephthegeek

Offline

Joined: 10/20/2007

Juice: 575

Re: Re: Re: Ryan wrote:Hey Steph, was

Woohoo! For the first time I'm looking forward to styling the cart block again

One small thing, there's an errant period here:
<span class=".num-items">1</span>

And the additional span was simply because the arrow up and arrow down spans are inside the title... so the other layer was for easier targeting of *just* the title text. But it's probably best to keep that (already complex) area lean, and you can always undo styling on .cart-block-title-bar span {} if necessary anyway.

Edit: hey cool, I have a badge

Gorgeous original Drupal themes (and Ubercart themes!) ~ Psst: more Ubercart themes on our new site

Wed, 02/18/2009 - 09:28

#16 View page

Ryan

Offline

Joined: 08/07/2007

Juice: 15438

Re: Re: Re: Re: Ryan wrote:Hey Steph, was

hehe You are indeed a Theminator.

Will fix that errant period and get this baby committed. I don't have a problem w/ the extra span, so we can always drop it back in if need be. Lemme know if it ever starts being a pain. Three cheers for a cart block that isn't awful!

	User login
	Log in using OpenID: What is OpenID? Username or e-mail: * Password: * Remember me Log in using OpenID Cancel OpenID login Create new account Request new password

Ubercart

Navigation

User login