GVS recently did some usability testing on a client site that included Ubercart and UC_Signup, and identified potential usability issues that apply to Ubercart. I'm posting those here so others can weigh in on the proposed improvements.
When viewing their cart contents two of four participants clicked the "Remove" box next to the item, as if to confirm the selection.
Potential Improvements
- Replace the remove checkbox with a "remove" button, like amazon does (this eliminates the need to also click "update cart" when changing removing a single item)
- Move the checkbox to the left of the quantity field, so that it is more visually connected to the idea of quantity.
- Call attention to the purpose of the box by a question mark so it reads "Remove?"
Comment | File | Size | Author |
---|---|---|---|
#98 | 529110_cart_item_remove_button.patch | 4.49 KB | Island Usurper |
#94 | remove-product-button.patch | 4.44 KB | Bartezz |
#92 | 529110-cart-remove-followup.patch | 2.87 KB | longwave |
#82 | 529110-82-cart-remove-button.patch | 3.75 KB | fenstrat |
#75 | 529110-cart-remove-button-3.patch | 3.98 KB | longwave |
Comments
Comment #1
rszrama CreditAttribution: rszrama commentedI had a similar thought when looking at Lullabot's old Shopify store. "Unfortunately," they've switched to Ubercart, so I can't reference it any more. Will have to find another Shopify store... their cart page absolutely rocked. : P
Comment #2
dman CreditAttribution: dman commentedA checkbox to signal deletion is a UI wrong. Only webmails or phpmyadmin can get away with that.
On other things - (like the Drupal node-edit "menu" widget) it's backwards :-B
That said .. A button makes it hard to do more than one thing at once.
Maybe we need AJAX. It actually IS a checkbox for degradation but UI script replaces it with a button and action?
Comment #3
Zach Harkey CreditAttribution: Zach Harkey commentedThe uc_ajax_cart module provides this exact functionality and it works awesome.
Unfortunately it also hijacks the 'add to cart' button with an unhelpful ajax variation that updates the cart total without leaving the product page. While this sounds good in theory, in practice the user can't tell that anything has happened and they just continue to click the button, which adds more and more products to the cart. The user then has to locate the View Cart button somewhere in the navigation or cart block.
I much prefer the default method of sending the user directly to the shopping cart. In fact, I don't see why they couldn't just go straight to checkout.
Comment #4
echoz CreditAttribution: echoz commented+1 on - Replace the remove checkbox with a "remove" button, eliminates second step clicking "update cart" to remove item. As a user that is not confused what the checkbox does, I'm always impressed by a cart that lets me remove with one click.
Comment #5
JurriaanRoelofs CreditAttribution: JurriaanRoelofs commented+1 on moving any deletion control close to the quantity indication.
Comment #6
justinlevi CreditAttribution: justinlevi commented+1
This is a really poor default UI element
Comment #7
xurizaemonAttached is a module which implements functionality like this. The code isn't perfect, and I'd really appreciate feedback on a couple of points.
The main challenge for me was making the remove button (which I've made an image_button) store and return the data pointing to which item in the cart should be deleted. I think that some aspect of the tapir table and form generation code was confusing me - I added a series of elements of the form
$table['remove_button']['remove_button_0']['#value'] = 0;
but the returned value was always the value stored in the final button on the form.Anyway, hacked around that with an ugly function which inspects the posted data and does the dirty work.
I would be really happy to rewrite this for inclusion in uc_cart, rather than as an external module, so it can be presented as an optional method for removing items. For some shops, this makes a lot more sense. Others may prefer to keep the current setup. Some crazy folks may even want both onscreen at once (now, THAT would cause misunderstanding).
(Also, I should make the remove button a bit more themeable - input welcome on the best way to do this - but CSS should work fine for now.)
Comment #8
xurizaemonAnother issue which needs to be resolved before this is ready for primetime is that changing the Qty of one item, then deleting another will revert the changed Qty of the first.
Screenshot of the remove button placement below -
Comment #9
xurizaemonUpdated code for this module. Really keen for some feedback on any improved methods to achieve the same result.
Comment #10
torgosPizzaIf you want this to be included in core, you should roll your changes into a patch :) I doubt it'd make it into 2.0-stable, but maybe the Uberdudes would be keen to roll it in later on? It does seem to make more sense than a checkbox.
Also you can AJAXify the cart table so that when a cart is removed or updated the user never has to see the page refresh. And of course it'd degrade so that without JS the page does reload.
+1 from me for getting this into core.. at some point!
Comment #11
xurizaemon@torgos - I understand, but this code is miles from being ready to merge into core.
In particular there's the issue raised in comment #7 which I had to work around using some hella ugly code to inspect which remove button was clicked.
Maybe you can point me in the right direction. In short -
* We set a custom submit handler
uc_removebutton_remove_item()
for the remove button.* We set a value on that image submit button, which is the index of the cart item to remove.
* But in the custom submit handler, I found I couldn't extract the value using
$id = $form_state['clicked_button']['#value'] ;
- this would ALWAYS return the index of the last remove button on the form.* So instead I've got a very ugly
_uc_removebutton_identify_removed_item()
function to pull the clicked button out of the post values.I'm sure there's a way to make this work. If I can turn this into code that I like the look of, I intend submitting a patch to UC.
Comment #12
torgosPizzaYeah I guess if you are being forced to use form elements it can get hairy. I'll give it a shot one of these days if I get really bored :)
Comment #13
torgosPizzaOkay, here's my first pass at it. I was able to do it in less code, and only had to hack 1 core modules to do it: uc_cart.module:
Add this to uc_cart_menu():
Add this block anywhere (I added it to the bottom so it was easier to find while writing it):
Add this to uc_cart_view_form(), inside the foreach($items as $item) loop:
And if you want you can adjust the weight of your columns within uc_cart_view_table($table) so that the "Remove" column shows up next to Qty. To do that I just made Remove have a weight of 4, and Total have a weight of 5.
Now keep in mind this is my first attempt, and from my testing it seems to work really well. I'd of course make the Remove link a button to call more attention to itself, or better yet have it tacked on underneath the Qty field, but this is as far as I got for now.
Feel free to give it a shot and if it seems solid enough to move into a patch I can do that. (I don't have the latest UC right now on my local testing computer or else I'd roll it against that.. plus I should check it out of CVS...)
Comment #14
torgosPizzaOh and here's a screenshot so you can see it in action.
Comment #15
valariems CreditAttribution: valariems commentedI found this issue page because I have received at least four emails over the last week since launching, where customers had their product "disappear" from their shopping cart. After speaking with one of them, she figured out that she had clicked the "box on the left" - the remove box - and then clicked the checkout button. Accidentally deleted the product. She thought that the checkbox was to confirm her product selection.
Would be interested in resolution to the issue. Unfortunately, I'm not a developer and can't write code to help. Was thinking some type of solution would be a confirmation popup like "are you sure you want to delete this item?" would be a solution. I'll follow along with the issue and see what happens - just wanted to chime in that my users were having issues.
Comment #16
willvincent CreditAttribution: willvincent commentedWould replacing the checkboxes with cartlinks links be doable? Their action could be ajaxified or not, but that seems like it might be a pretty straightforward solution...
A bit of additional code would be necessary to include some single item removal handling, but it seems like that would handle the issue to me.
For reference, because Ryan mentioned it previously, I've attached a couple screenshots of some shopify carts. Looks like the only other thing we're missing really is the price/item, rather than just a quantity and subtotal for each cart item.
I think it would make sense to add a column for item price. So if I were ordering 5 baseballs that cost $3 each it wouldn't say Qty 5, $15.00 but rather it would say something like Qty 5, Price $3.00, Total $15.00
The other thing I noticed while browsing through some shopify sites is that their cart items aren't necessarily formatted as a table like ours, and the two I've referenced here, some had a separate div (I think, didn't look at the code) for each item, and price/qty/remove/etc could be anywhere in relation to the the title/photo of that item.
So ideally it would seem we need a complete overhaul of the cart (and probably the checkout and review pages as well) Most of the tables could be replaced with divs for each line item, with its associated items in their own div or span tags. The default css then could create table views as we have them now, but restyling the cart would be a dream for themers. I realize that's probably more of an ubercart 3 wishlist item, (and I'll repost some of these thoughts in the uc3 thread in the forum) but since we're talking about User interface/user experience stuff it's probably worth mentioning.
Comment #17
asak CreditAttribution: asak commentedJust tested out code by torgosPizza from comment #13 - it works.
I like tcindie's idea of adding an item price column - that could simply be added at that same uc_cart_view_form loop.
Getting rid of tables, in general, is always a good idea ;)
Comment #18
willvincent CreditAttribution: willvincent commentedWell, getting rid of tables isn't *always* a good idea.. they do still serve a purpose, and ubercart's usage of tables is justified more often than not. :)
Comment #19
torgosPizzatcindie: I personally like the option of having a unit price as an additional column in the cart. But that sounds like a separate issue.
+1 for getting a "Remove" link in there. :)
Comment #20
valariems CreditAttribution: valariems commentedI have implemented xurizaemon's module from comment #9, and it worked. No problems from my limited testing. I will know more when users start shopping.
I could not get the hack from torgosPizza's comment #13 to work. My problem was when the "remove" link was clicked, the result was the 404 error page, and item was not removed. However, my php skills are all but nonexistent, so it is very possible that I was putting the code in the wrong place or otherwise messing things up.
I'd actually prefer a "remove" text link to the red button, only because my site's users have continuously amazed me with their lack of online shopping skills, and I worry that they still won't get the meaning of the button. But I'm still happy to go with the graphic for now.
Big thanks!
(also posted in ubercart forum thread)
Comment #21
torgosPizzaValariems, you probably needed to go to the "modules" page to rebuild the menu router table. Since the code I wrote adds a new URL to the site, if the site doesn't know the URL is there, you'll get a 404 page. In my testing it worked without any problems. And the nice thing is we can make it a text link by default, but with CSS you could easily change it to a button if you prefer.
Comment #22
patcon CreditAttribution: patcon commentedAlright, so not sure if ubercart changed a bit, but I think something needs clarification for those as dim as myself :)
Make sure you put the code inside the
if
section of the foreach loop, like so:I'm sure most would assume that, but apparently I'm a little slow. If you're seeing a checkbox with a remove button in a blank row below each product, then you probably pulled a "me". I originally had added it after the
i++
line! Oy... Cheers allComment #23
Vivekdrupal CreditAttribution: Vivekdrupal commentedThis code in comment #13 does not work when we have product kit as a cart item.
I tested this code with my project. If i am successful doing this for product kit also, i will post the code here.
Vivek
Comment #24
TR CreditAttribution: TR commentedTagging
Comment #25
patcon CreditAttribution: patcon commentedFYI, I've run into this problem a few times as well -- customers call me saying the cart is broken, and that every time they try to proceed to checkout, the shopping cart empties.
Comment #26
TR CreditAttribution: TR commentedIf someone will create an actual patch I'm willing to put this feature into Ubercart.
Comment #27
TR CreditAttribution: TR commented#471106: Update the cart form "remove item" workflow also discusses this topic, and contains some very relevant suggestions including images of how "remove" is handled by a number of benchmark e-commerce sites. I have marked that issue as duplicate, because this one seems to be further along toward a solution, but please consult that older thread when making a patch.
Comment #28
ahimsauziThanks Torgos!
I can't understand why the remove checkbox won't work out-of-the-box but your code worked for me.
Comment #29
Rainman CreditAttribution: Rainman commentedsubscibing
Comment #30
gausarts CreditAttribution: gausarts commentedSubscribing. Thanks
Comment #31
lunk rat CreditAttribution: lunk rat commentedSubscribing
(I once deleted stuff out of my cart unintentionally via those silly checkboxes; granted, I was drunk, but still . . . I have had client complaints about this.)
Comment #32
SeanA CreditAttribution: SeanA commentedLooks good torgosPizza! Here's #13 as an actual patch. ;-) Rearranged the cart columns and added a question mark as suggested.
Comment #33
torgosPizzaKiller! This is one of those things that is sort of a "duh" and deserves to go into core. Hopefully TR can revisit this issue and put it in! I'll test out the Patch on some of my installs and update here.
Comment #34
torgosPizzaTesting this on our live site which still uses 6.x-2.2 and it works like a charm. I hope to update to 2.4 this weekend, and I assume it'll still work then as well.
I haven't tested it with Product Kits but based on my experience from earlier this code doesn't work with them yet. Going to need to figure that part out, maybe in a separate patch.
Comment #35
smscotten CreditAttribution: smscotten commentedAdded patch in #32 to my 6.x-2.4 install and it seems to work exactly as described.
Comment #36
longwaveThe problem with this solution is that the remove link is a GET action, but it should be a POST. Otherwise there's the possibility of a CSRF attack (see the Cart Links security issue for a similar problem), and also that some browser configurations try to prefetch links in order to render subsequent pages more quickly - however in this case, any prefetch would result in items being removed from the cart, which isn't what the user intended.
Comment #37
torgosPizzaAh, okay. So a better solution is to make this a tokenized form with a "Remove" button. I'll see about adjusting the patch to provide that instead.
EDIT: It looks like the module / code in #9 and #11 are more along these lines... I'd like to give them a shot, at least as far as merging those changes into a patch for core. Will see what I can come up with.
Comment #38
longwaveUnfortunately, the problem with that approach is that the remove checkbox/link is already part of the larger cart form (which is still required so people can update quantities of multiple items at once), and I don't think you can successfully nest forms inside one another.
A workable solution which I've used before is to employ JavaScript to hide the checkboxes and add "remove" images with click handlers that check the hidden checkbox and submit the form for you.
Comment #39
longwaveActually instead of nesting forms, you could maybe add a "remove" submit button on each row instead of the checkbox, and then detect whether the user clicked a "remove" button or the "checkout" button in the submit callback.
However, I think we may still need to keep the original checkboxes approach for some stores - say you have many items in your cart and want to remove some or all of them using the cart page, you don't really want to have to click and wait for the page to reload in between removing each item.
Comment #40
torgosPizzaYour thoughts in #39 is sort of where I'm headed with this. Just creating another submit button, which I can hopefully attach a callback to, or will otherwise add the 'remove' item to the $cart->item object, which in turn triggers the "remove" functionality that's already part of the module.
Will try to keep digging on it.
Comment #41
TR CreditAttribution: TR commentedWhat do you think about making this AHAH instead of a plain link?
When making the patch, please remember to consult #471106: Update the cart form "remove item" workflow, especially the screen shots of other carts that Steph provided. I think the solution in #32 is ugly - Product name is not left-justified (looks like the checkbox column was not removed from the table body?) and the remove link is too large and in the wrong place.
Comment #42
torgosPizzaYeah, I'm starting to think AHAH is probably the answer... use jQuery to replace the checkbox with an image or a text link, which just fires an AHAH POST event. That would make life a lot easier, because as grobot discovered, using a submit event within that form will just call the id of the last button. It's really aggravating and after some research today, I don't think there's a more elegant way around it. (Question for others more intimately familiar with FormAPI: Is this an HTML thing, a FormAPI thing, or a Tapir thing? By "thing" I mean the way it only sends the id of the last button to the submit handler.)
It's a shame, because it almost works... there's just no easy way to find out exactly what button was pressed in the submit handler (even creating a custom callback) unless there's something I'm missing with post-processing functions or something.
I even thought about duplicating the "Qty" field, because you can kill the "Remove" op and replace it with a "qty=0" dedicated button to achieve the same effect, but you still run into the fact that it's a button.
I can't spend anymore time on it today but I'll think a little more about the AHAH aspect of it. I'd like to figure out a way around this without resorting to that, but if it can't be done, it can't be done.
Comment #43
SeanA CreditAttribution: SeanA commentedRe #41: the tables columns don't look properly lined up because there was no product image on my test product. The space to the left of the product name is where the image would be. (The fonts are large because the minimum font size is large in my browser, I don't like trying to read tiny fonts.) Anyway, I'm not attached to that layout, but I do think the 'remove' button should be associated with the quantity field, either to the left of it, or directly beneath it.
After mulling this over, it occurred to me that using javascript to replace the checkbox with a 'remove' button would be a good approach. When the user clicks 'remove', the hidden checkbox is checked and the CSS style on that row would change to indicate the item was removed. Also a message would appear at the bottom of the form, something like "Changes made will not be saved until the form is submitted". (A UI approach taken by TableDrag for rearranging blocks, menus, etc.) The item(s) wouldn't actually be removed until the user clicks the "update cart" or "checkout" button.
One advantage of having it work like this is we could then have a 'restore' button on removed items that allows the customer to change their mind and return an item to the cart.
Then I realized that CCK3 (multigroup) has similar "hidden checkbox/remove & restore an item" javascript for the multigroup node input form, which looks like it could be adapted as a solution for the shopping cart. Check it out: content.node_form.js. Disable dragging and rearranging, but retain the remove/restore stuff.
Comment #44
derjochenmeyer CreditAttribution: derjochenmeyer commentedI like Patch #32. It works as expected for ubercart 2.4
This slightly improved version just fixes Issues related to the correct use of t().
What about using check_plain (CSRF attacks)?
EDIT: If I can choose, I will always choose the option wihtout AJAX. Maybe the above approach can be AJAXified like Views does it. UI Option: "Use AJAX to remove items form the cart."
Comment #45
derjochenmeyer CreditAttribution: derjochenmeyer commentedNew updated patch with small modifications.
Comment #46
torgosPizzaI think to avoid the "prefetching" issue mentioned in #36, the "Remove" link should be edited to have
rel="nofollow"
added to them. That avoid avoid that from happening, I think.Also maybe the links should be tokenized.
Comment #47
xurizaemonForgive me if I misread your last comment here torgosPizza, but ... If you're thinking of going the route of including a regular link with nofollow and hoping that nofollow will prevent prefetching, I'm not sure that will be a robust solution.
Nofollow is an attribute intended for Search engines. I don't believe we should depend on it to prevent browser prefetching from having unexpected consequences on cart actions, because that's not its stated purpose. Removing an item from a cart is a state change and therefore suits a POST request.
Drupal should be capable of identifying which "remove" button was clicked. When I posted in #7 - #11 and mentioned that there were issues identifying which button had been hit and which item to remove, I was unsure where the problem sat (my use of FAPI, Tapir, or FAPI itself) and asking for comment from others. I haven't been tracking patches on this issue actively since then, but (as longwave flagged previously) I don't believe GET is the correct solution, and that's what the most recent patch does.
If we do removals via AJAX (+1), I don't see why we'd introduce the "Changes made will not be saved until you do secondary action X". Excepting that it's consistent with Ubercart UI elsewhere, this seems like it introduces additional uncertainty in the mind of the user, and we should be as gentle as possible with those poor souls in checkout as they navigate the scary process of getting out their wallets and handing over the moolah. (That is the is the ultimate goal of this ticket, right?)
Marking "needs work" but I want to stress that I've only read the patch and established that it uses GET to provide remove buttons, I haven't actually tested it.
Comment #48
derjochenmeyer CreditAttribution: derjochenmeyer commentedDrupal identyfies which button was clicked by checking the value e.g. t('Update cart') or t('Checkout'). See: uc_cart_view_form_submit(), uc_cart.module, around line 1020.
If we use a remove button it will have the same value for every item e.g. t('Remove').
I agree that using POST is the better approach. But I dont see how this can be done without rewriting a lot of existing functionality.
Comment #49
torgosPizzaBut I dont see how this can be done without rewriting a lot of existing functionality.
grobot was able to do this, but with a bit of a kludge. I didn't get to test out his module, but I didn't think that was a bad approach either. I think I accidentally stepped on grobot's toes when I posted my patch after ignoring the code/module posted in #9 and #11.
@grobot: Yeah, I didn't think nofollow would be the answer, which is why I was hesitant to write it. I agree that POST is the best solution and the only real way I can see that happening is with an AHAH/Ajax call. In this sense I kind of like the idea of keeping the checkboxes there and if a user has JS they can automatically be hidden and replaced by an Ajax removal button that can post to its own callback URL.
Comment #50
xurizaemonMy toes feel untrodden, I'm just delighted this is getting followed up :)
Can anyone answer these questions for me - any/all of which might suggest approaches we can take?
If we set per-element validation/submit handlers, do they not get information passed in about which element was submitted? (I thought this must be possible at the time and ascribed my inability to do so to lack of FAPI voodoo skills. This is what I was trying to do in #11.)
If we use submit images, can we get any information about which image was clicked? (I don't think we do, I think I tried this but it's been over a year since. Again, see comments on #11.)
#48 says,
I'm not sure that's necessarily true? We could customise the 'Remove' message to pass info back to the submit function. Eg we could replace the checkboxes with a submit button (or image) with text "Remove 3 x SKU-123"? (This doesn't handle use case where we have 3 red SKU-123 and 3 blue SKU-123 in the cart, but you get the idea - embed the cart line item info in the submitted value, then conceal the ugly button via JS for the AHAH version.)
Comment #51
xurizaemonThe only issue AFAIK with the kludge in #11 is that we rely on $_POST and so we lose FAPI programmability. This *may* not be an issue because use cases for cart editing by form programming are fairly limited (tests only, I'd guess). So let's not discount that approach.
I had assumed there was a clean FAPI method for doing what we need, but if $_POST is the only way, and it doesn't hurt us much, let's not rule it out. (And if there's really not a clean FAPI way to do this, let's start rocking that tree until we have one.)
Comment #52
rszrama CreditAttribution: rszrama commentedHey guys, I just had to deal with this in the Commerce Cart module (we're using submit buttons w/ a touch of styling [not implemented yet]). If you set the #name property of the form element to something unique, the FAPI will pick up on which one was actually clicked. See the code in this post:
http://drupal.org/node/916348#comment-3518900
Comment #53
torgosPizza@Ryan:
Say whhaaaaat?? I thought we were already doing that in UC... I'll revisit my code tomorrow and see if I can use it to rework what I started with using a button. You're sure that if you the change to #name it will only return the clicked button, and not the last button? (Which is always the very last "remove" button in the table.)? If so, that's freakin' sweet!
Comment #54
derjochenmeyer CreditAttribution: derjochenmeyer commentedHere is an approach using POST and the name attribute (name="item[CART_POSITION][NODE_ID]").
Comment #55
torgosPizzaGeeez. That was almost too easy. I wrote and tested the patch in about 15 minutes, and it seems to work fine. I tried to make the minimal changes to get it to work - please give it a look and test it out.
(This is against 6.x-2.4.)
(Thanks, Ryan!)
EDIT: Oops, sorry derjochenmeyer, I didn't see your post.. damn you are fast. :) I'll test out your patch.
Comment #56
torgosPizzaOne thing I'd like to mention is that I'm not using a line item ID, which maybe we should (the patch posted above mine seems to do so, which is probably better). The current patch uses the $nid, which *might* cause problems with product kits that are being shown as a "single unit" and not "individual products". If someone could test that part out that'd be helpful.
Also, I need to wrap the "Remove" text for the button in a t(), and I want to make sure that it's styleable/themable enough that someone could replace it with an "x" image if they wanted. Perhaps another theme() function? Suggestions welcome.
Comment #57
derjochenmeyer CreditAttribution: derjochenmeyer commented#55: now I see :)
Displaying a message would be a nice addition.
Comment #58
torgosPizzaHmm, a couple things I don't really like about the patch in #54:
- Putting the "remove" button at the far right. Most shops (at least in English-speaking territories) have it on the far left. Amazon is a good example of this, but perhaps we can make this a "Weight" setting in the Cart settings page?
- I don't like having to use a callback to process this - since we're already able to use existing functionality. It's easy enough to add the message to uc_cart_update_item_object().
- Again, in the callback, using node_load() to find that stuff seems unnecessary, since the module already does a lot of this lifting for us.
I'm going to try redoing my patch with some ideas from the other patch..
Comment #59
torgosPizzaOkay, here's an updated patch that adds a message and fixes the redirect (it brings you back to the cart). I'm still using the $nid and not the $i (line item id) but I'll wait on some feedback with Product Kits before I know whether or not we have to do it that way.. can't test them on my dev site at the moment.
EDIT: correction, I was able to test it with the one remaining product kit we had, and it worked perfectly.
Comment #60
SeanA CreditAttribution: SeanA commentedTested #55 and works as advertized. Here's an updated patch with a different approach than #59 for redirecting back to the cart with a message. Makes use of the existing switch statement. (#59 appeared while I was working on this.)
Personally, I like the Remove button all the way to the right, tucked out of the way. So yes, I think a config item for setting the column would be nice.
And... thanks guys for working on this!
[edit] - I made a patch for what was discussed in #16 about displaying a column for the unit price. Creating a new issue.
Comment #61
longwaveI don't think you can rely on $nid alone, as you may have multiple items with the same nid but different attributes in the cart.
Comment #62
torgosPizza@longwave:
Good point, I hadn't thought of that. I'll modify my patch - or maybe the one in #60 - in the morning to use a line item as well (or in place of). It'll probably mean a little more work in the update_item_object function but should be a minor conditional. Unless SeanA wants to update his patch first. :)
I think the two things we should add into the patch are:
1) Make sure it works with multiple variations on the same $nid (so use the line $id instead).
2) Configure a weight field for the placement of the Remove button.
Comment #63
torgosPizzaOkay, here's my updated patch. (Sorry, I'm bored tonight and feeling productive, even though I should probably be in bed!)
Takes care of issue #1 by using the row # as the item appears in the cart list, plus the $nid.
Takes care of #2 by adding a new field into the admin/store/settings/cart/edit form.
I also added back in the drupal_set_message() to the "Remove" process. I think, as part of a good user experience, the customer should know what product they just removed from their cart if they accidentally hit the button, so that they can get it back if they want. (Maybe we can add an "Undo" link to the message in the future.)
Test at your leisure. Works fine in both my prod and dev environs.
EDIT: (Note): One thing I don't really like about the way the switch() is looking for the cart process is the way it uses t():
.. how does that not cause issues in multilingual sites? Seems we should use something that is standard, like the #class of the form item, or something. Anyway we can patch that later, as it's out of scope of this issue.
Comment #64
derjochenmeyer CreditAttribution: derjochenmeyer commented#59, #60
Thanks for working on this feature. Some small additions:
The form item ist created in hook_cart_display() (ubercart/docs/hooks.php). Maybe it should be changed there indstead of overriding it in uc_cart_view_form().
$element['remove'] = array('#type' => 'checkbox');
Like this it is not translatable, because you have to translate the string for every (!) product. Correct use of t():
Something like this.
EDIT: #58 I added the node_load() in the callback because the function uc_product_is_product() will do a node_load if only the nid is passed through. But anyway the new approach is much better :)
Comment #65
derjochenmeyer CreditAttribution: derjochenmeyer commented#63, thanks for the patch.
This could be a seperate case with a better feedback (message) for the user what actually happened.
Maybe its possible to use the key/value pair instead of counting?
Comment #66
torgosPizza#65:
Your points are valid. I'll see if I can update the patch today, unless someone wants to beat me to it. (I'm back at the office so I have other priorities at the moment.) Thanks for the feedback!
EDIT: I'm looking into moving the "Remove" $form item into uc_product(_kit)_cart_display() - the big issue here is that you then lose the cart id of that particular line. All we have left is the $node info and the $cart->cart_item_id. That could be useful, except the entire cart update process relies on the $node->nid based on that particular module's uc_cart hooks invocations.
Make sense? In other words we need to edit either, 1) the way the cart is built or 2) the way hook_update_cart_item() works. Both of these seem to be rather large API changes, and I'm not sure that's something we want to do right now.
Comment #67
SeanA CreditAttribution: SeanA commentedA few thoughts.
1) It seems that we can just use the item number by itself for the button name:
'#name' => 'cart-delete-item-'. $i
2) Code cleanup. In the original, we have this:
Which looks "messy" to me. This seems cleaner and clearer:
So with the Remove button stuff, and using just the item number, it would look something like this:
3) Adding a class for easy styling. This can go in uc_cart_view_table where the other elements are assigned classes.
4) A "weight" for the Remove button. Ideally this should be generalized so that shop admins can rearrange all of the columns in the cart display. Which sounds like it should be a separate issue.
5) In uc_product_cart_display (hook_cart_display in uc_product.module) we can get rid of the checkbox element.
Comment #68
torgosPizzaSeanA,
Thanks for the feedback. We could probably actually change your last hunk to not count, either, as proposed a couple posts up:
If you can write a patch I'll give it a whirl, otherwise I'll try to modify mine soon.
Comment #69
longwaveI am not convinced the item number alone is reliable enough; if the user uses the back button to get to a cart page in their browser cache, or has the cart open in one tab while adding/removing products in another tab, there's a chance they will click a remove button and it will remove a different item to the one they're expecting. The uc_cart_products table has a primary key (cart_item_id) that should probably be used here? Although it seems that this id isn't really used in Ubercart, uc_cart_update_item_object() relies on the index anyway.
In fact, instead of modifying uc_cart_update_item_object() so it is passed a button, shouldn't that function be left just to update quantities and the "remove" handling put straight into the uc_cart_view_form_submit() - that would seem to make more sense, as uc_cart_view_form_submit() is already responsible for dealing with other button clicks in the cart.
Comment #70
longwaveActually, using the index should be fine, as the array was built by FAPI and the item number is guaranteed to point to the correct data. My attempt at a patch is attached. I've deleted the 'remove' checkbox from hook_uc_cart_display as I think item removal should be handled consistently by the cart itself, rather than whatever module (probably uc_product) is responsible for displaying the cart item.
Reordering the cart columns was possible with the TAPIR interface back in the days of Ubercart 1.x; the hooks still exist for this but the UI is gone in Ubercart 2.x for some reason. Perhaps this should be reintroduced, rather than just adding a single setting for the remove button weight.
Comment #71
longwaveUpdated patch attached that includes a drupal_set_message() in the same style as the "added to your shopping cart" message and removes the unnecessary submit handlers from all the cart buttons; uc_cart_view_form_submit() will always be invoked anyway.
A CSS class for the button itself isn't required, you can use
#cart-form-products td.remove input
to target the buttons.Comment #72
longwaveI've also just realised how horribly convoluted cart item removal actually is:
uc_cart_view_form_submit() calls uc_cart_update_item_object() to update item quantities
...which invokes hook_update_cart_item for each item
...which, in uc_product's case, updates the database (for quantities > 1) or calls uc_cart_remove_item()
...which invokes hook_cart_item op 'remove' and deletes the row from the database
This probably should be looked at for Ubercart 3.x as it doesn't really seem the ideal way of doing things!
Comment #73
SeanA CreditAttribution: SeanA commented#71 doesn't seem to be working. It would be a nice shortcut though! What happens is the last item in the cart is removed no matter which Remove button was clicked.
Here's an update of #63 that uses cart_item_id for naming and identifying which Remove button was clicked. Not sure if it's the best way, but it seems to be working.
Styling: the 'remove' item already is getting a class assigned in uc_cart_view_table (from when it was a checkbox). So as longwave pointed out, we don't need to do anything there.
Comment #74
torgosPizzaSeanA,
I think I like your approach the best. Other than lacking a weight field for the Remove button (which should probably be its own issue), it looks solid to me. I'll test it out asap.
Comment #75
longwaveAlternative version of the above patch. I don't see the point in changing the signature of uc_cart_update_item_object() and passing $clicked_button if it's not needed.
Comment #76
fenstrat#75 works a treat, and with no api change. I'd RTBC it however it's a pretty big change to apply to 2.x.
As ugly as it is, there's got to be modules / custom code relying on the existing checkbox behaviour?
Comment #77
TR CreditAttribution: TR commentedI don't think there are many modules or custom code relying on the existing behavior. I'm inclined to commit #75 unless I hear of some negative consequences.
Comment #78
fenstratOk, #75 is ready to go.
Comment #79
longwaveI think this change should explicitly be listed in the release notes of the next version, just to make users aware that they might have to revisit any custom code that alters the "remove" behaviour of the cart form; as I said in #38 I've written custom JS to simulate this change before now, and I'll have to update that after this patch is committed. But let's at least get this committed to -dev, and hopefully we will see any bug reports or further feature requests before the next full release if this change causes problems.
Comment #80
torgosPizzaAgree with longwave, mention this explicitly (I would even consider a hook_update that sets a message to the admin making them aware, but not sure if it's worth that much effort) and I think we're good to go.
I'm glad we are finally getting this in. Nice work everybody! (And glad I could help, to an extent. ;) )
Comment #81
SeanA CreditAttribution: SeanA commented#75 works as expected. Thanks everyone!
Comment #82
fenstratThis is a re-roll of #75 against -dev as there was some offset.
The only change is that I've added cart_item_id as a hidden field in uc_cart_view_form. As longwave points out in #69 cart_item_id is not used in Ubercart core, but it's a useful value. Setting it here saves having to work it out in contrib form alters, and it's no overhead to add in here. I'm reworking uc_save_for_later for 6.x and it's certainly needed there.
I agree with #79, this change needs a prominent listing in 2.5's release notes.
Leaving as RTBC.
Comment #83
longwaveRTBC and no new comments for two weeks; time to commit this?
Comment #84
TR CreditAttribution: TR commentedI'm convinced that the patch is functional as described, but there has been little discussion of the usability issues so it's unclear to me whether anything was done along those lines since I last tested in #41. This issue has been caught up in the technical details of the patch, and hasn't gone back to see if the "new" way is any better from the user's perspective than the old way. An image or two posted here would go far by showing what the proposed cart looks like.
Some outstanding questions - did we lose the ability to set the weight of the remove button as implemented in #63? Can the remove button be easily themed, e.g. by replacing the button with an "x" image as in #45? What about moving the button underneath the Quantity box? Ezra's original post spelled out a number of key issues, but only the first (button instead of checkbox) has really been talked about. Steph's screenshots of carts were great - have any of the patch creators looked at those? If so, which layout is the patch using and why? The whole motivation of the patch is usability, so I'd like to make sure that gets fixed otherwise we'll all be back again to do it over. It's quite possible all these things have been addressed, but I've seen no explicit discussion and no images so I have been planning to try out the patch myself to get these answers. However I've been a bit short on time.
Comment #85
torgosPizzaYou're right, looks like no one followed up my admin.patch and all of the subsequent button patches have lost the addition of the 'weight' value. It should still apply, but the button patch will have to be updated to include the admin setting's weight value from that patch.
As far as usability goes, I've been using these patches on our production site with no problems. Haven't had feedback from users, but myself when I've been testing it, they have been pretty helpful as far as getting the idea across goes. I think the checkbox always felt a little strange, seeing as how most other online shops use a remove button or link.
Comment #86
james.wilson CreditAttribution: james.wilson commentedApplied patch from #75 and it works perfectly.
I definitely think this should make it to ubercart core.
Comment #87
stefan81 CreditAttribution: stefan81 commentedPatch #75 works well!
Comment #88
SeanA CreditAttribution: SeanA commentedThere are no outstanding questions about this patch.
- As discussed, setting the weight is a separate issue. If we're going to allow setting the weight of one column in this table, the proper way forward is to allow setting the weigfht of every column. A separate issue should be opened for this.
- As discussed, the button is easily themed.
- Usability. The patch fixes a bad problem with having checkboxes to remove items. If shop owners lose even a single sale from the checkbox method, it's a Very Bad Thing.
#75 should be implemented immediately.
Comment #89
longwaveI honestly don't understand what is happening regarding fixing and improving Ubercart. If this were committed to -dev it would receive more testing and any undiscovered issues can be ironed out. If it proves to be a mistake for some reason it could be rolled back or a config option introduced to enable/disable it as necessary.
See also #1001164: Offering to become co-maintainer of Ubercart 6.x-2.x - I am more than happy to work on any subsequent issues that may arise from committing this and a number of other RTBC patches that are currently in the queue; I may be wrong but I hope that other developers would also be more likely to submit patches if they saw more activity in CVS and the issue queue.
Comment #90
Island Usurper CreditAttribution: Island Usurper commentedCommitted #82, since the cart_item_id is probably useful to contrib in this case. I'm sorry for the lack of attention this patch has gotten.
Comment #91
fenstratThanks for committing #82 (and other RTBC issues) Lyle.
I'd still really like to see longwave become a co-maintainer.
Comment #92
longwaveFollowup patch attached that fixes removal of product kits and moves the "remove" button back to the implementing module, which adds backward compatibility for other modules that handled their own cart items.
Comment #93
longwaveAlso needs porting to 7.x.
Comment #94
Bartezz CreditAttribution: Bartezz commentedI couldn't apply parts of the various patches against my 6.x-2.4 version.
Applied #75 and #92 by hand and created a new patch.
As nothing has changed in the code and I found both original patches working I'm marking this as RTBC.
Cheers
Comment #95
longwave@Bartezz: #92 should apply against the current 6.x-2.x-dev version, #75 is already in the -dev release.
Comment #96
Island Usurper CreditAttribution: Island Usurper commentedCommitted #92. Thanks, everyone.
Comment #97
Island Usurper CreditAttribution: Island Usurper commentedPatch combines the two patches from #75 and #92.
Comment #98
Island Usurper CreditAttribution: Island Usurper commentedUhh...this patch.
Comment #99
longwaveReviewed and tested with products. Product kits don't seem to be working in 7.x at the moment, I don't see the option to create one, but I think the patch should be good for these anyway. Committed.
Comment #100
longwaveComment #102
Anonymous (not verified) CreditAttribution: Anonymous commentedI've attempted to use hook_form_alter() on the cart to override the form buttons with images with this code:
Of course it replaces the buttons just fine. However clicking these buttons (as well as the new 'Remove' button) always takes you straight to checkout regardless of the settings in the Ubercart configuration. I've attempted a few different things to find the problem such as the incredibly handy Late Form Alter module to no avail.
Oddly enough, overriding the 'Continue Shopping' button and the 'Checkout' button [i]still[/i] breaks the untouched 'Update' button and sends you to checkout. That alone makes me thing I'm doing something incorrectly but I'm puzzled what it could be. Any thoughts?
Comment #103
TR CreditAttribution: TR commented@NANERPUSS: Please open a new issue. Your problem is not the same as this (closed) one.
Comment #104
Anonymous (not verified) CreditAttribution: Anonymous commentedHmm well this didn't happen when I tested it on a stable 2.4 version, hence re-opening this issue. I'll do that later though, thanks.
Comment #105
hanoiiProbably got here a bit late.. but.. what leave for the ones we liked and want/need the checkbox. Why wasn't this add maybe as a setting? Worth re-adding it? First client I installed 2.6 in requested the checkboxes (not knowing it was like this on previous versions).
Comment #106
bancarddata CreditAttribution: bancarddata commentedI would like to reiterate #105 by hanoii. I understand this is a usability improvement for users who are not very observant, but with this new "feature", has anyone tried to clear a cart with 20 items in it? It now takes like 3 minutes to do this because you can only remove one thing at a time, or you have to set everything to qty 0 (0, tab, tab, tab, 0, tab, tab, tab, 0, etc.).
A few possible suggestions:
I am not sure which is best, but I do not think the solution as it stands currently is truly the easiest way. At least a clear cart button?
Comment #107
james.wilson CreditAttribution: james.wilson commentedNumber 3 is not a good idea.. I've labeled it better and it still did not fix the issue. People are used to the 'amazon' style of buttons. This patch was a huge success with our site.
1 and 2 are much better options, although I have to say.. if somebody 'mistakenly' adds 20 different products to their cart... they SHOULD have to click remove on each one. ;)
Comment #108
Anonymous (not verified) CreditAttribution: Anonymous commentedI'm going to have to agree with #107 on this one.
Comment #109
bancarddata CreditAttribution: bancarddata commentedI agree that #3 is not a good solution, but I do not agree that a customer should have to click remove 20 times. When one is dealing with product kits that are set "As individual products", all it takes is one click to get those 20 products in the cart. If its one click to get them in, it should be one click to get them out. It will be a common occurrence, at least in our case, for a customer to choose the wrong kit initially or change their mind later and want to go with a different kit.
I will start a new feature request - "Optional Clear Cart Button". That should work for everyone involved and should be pretty easy to implement.
However, I still would have liked this to be an option, especially since the functionality I want used to be there and now is not. I have looked at Amazon's cart and the only checkbox in the cart is the "This will be a gift" checkbox. I have never seen a checkbox designed to "confirm" a product that is in the cart; in fact I can't remember a checkbox in a cart doing anything other than marking for removal. I have done my fair share of online shopping, no doubt - perhaps I am old school. But, there are obviously a lot of people that feel the opposite of me and I have no intention of battling the masses if I am indeed in the minority (as it appears I am).
I looked at the issue comments more closely and saw reference to an AJAXified cart module, so perhaps that might help my needs. Like I pointed out before, one errant click to get 20 products in the cart shouldn't take 20 click-waits to undo. The clear cart button will fix that, and I think an AJAX version of the remove and update total would help make the process smoother than what it currently does now.