Closed (fixed)
Project:
Commerce Add to Cart Confirmation
Version:
7.x-1.x-dev
Component:
Code
Priority:
Major
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
26 Apr 2013 at 16:41 UTC
Updated:
5 Dec 2016 at 16:24 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
vasikeindeed, there are several issues CSS related:
- #1996724: Long titles makes checkout button unclickable
- #1975880: "Continue shopping" button unclickable.
- #1875946: Confirmation Window Is Cut Off Vertically
- #1856340: image displays on top of text
and probably others not reported yet.
here is a patch / first work on this:
- separate the CSS in 2 sections: layout and style
- fix the z-index value of the popup (> than product image zoom).
- re-work the elements positioning - need some testing (and probably some fixes).
things to be done:
- remove the style CSS section, all these aspects should be moved in Commerce Kickstart theme or Omega kickstart theme.
- also remove the positioning CSS from Commerce Kickstart theme or Omega kickstart theme - to avoid conflicts.
Comment #2
Christopher Riley commentedMaybe it is a conflict with other styles but after this patch it is actually worse.
Comment #3
vasike@cmriley : as i said the patch requires other changes.
what about testing on other theme than Commerce (and Omega) Kickstart theme?
Comment #3.0
vasikeLet's restructure the sentences :-)
Comment #4
philsward commentedGoing to throw out my $0.02 on this since I spent an hour trying to fix the current state of the CSS to get it to look right on a site.
Obviously, it needs some CSS love. Don't need to point that out.
What I do want to point out, is the use of CSS on the "buttons", specifically the "checkout" and "continue shopping" buttons.
I've noticed recently that a lot of folks will create a styled "button" using the div containing the link, then maybe apply a hover to that div to get the onhover action going... there's two problems with this approach and I can't believe I see it done A LOT.
1) applying a hover to anything other than an anchor (a tag) isn't backwards compatible. I don't think it was until IE8 that this was allowed. Not as big of a deal, I know, but the next problem is bigger...
2) when doing the above approach, a button is never actually created. All that's done is making a box around a link and then hovering the box to make it look fancy. You can't click on the box to take you somewhere. That's where the problem lies. If a button looks like a button, it should be clickable like a button. What bothers me most about the current approach of the CSS on this module is that the pointer changes to a finger when hovering over the box. You can't click the box, but the pointer tells me I should be able to. The ONLY thing that can be clicked are the actual anchor links: "Go To Checkout" or "Continue Shopping". Horrible UX.
The best approach to creating a true button (outside of an image which is so a decade ago) is to style the button at the anchor level, not the div level. It does help to make the leading div designate it as a button though.
Here's some rough code example of what works:
HTML
CSS
Drop this simple code into the body of a Drupal page and you'll see how easy it is. Everything is applied at the anchor level making the entire anchor actually usable instead of just the anchor link.
Cool thing is, by taking this approach you can drop it in with your normal form input button styling to create really quick buttons anywhere on a site where a simple link button is needed instead of a form input, keeping your CTA similar across pages.
"Don't be a FaceBook. Usable Experience Web Design should Make Sense."
Comment #5
deggertsen commentedPatch in #1 wouldn't apply. Here's an updated patch.
Comment #6
deggertsen commentedHere's a version of the patch that uses the suggestions made in #4. I think this is what we should go with for now. I think we could still make some improvement by making it easier to override in individual themes. But I at least think it's a step up.
The only problem with this patch (though it could also be a benefit), is that it also addresses these two patches:
#2296685: Buttons do not close the window
#2346681: 'Go to checkout' goes to cart, not checkout
I usually like to keep patches exclusive to the issue at hand, but I had a hard time separating the issues.
Comment #7
deggertsen commentedAlright. I went ahead and separated out the code for the other two issues and made this one strictly design stuff specific to this issue.
I've added mobile support! There were no media rules so when you tried to view this on a small screen it looked awful. In order to do this, I did have to modify the html output slightly so that the buttons show up on the bottom instead of the top. I've tested this with a wide variety of screen sizes and it looks pretty good IMO.
I believe this patch fixes all of these issues:
#1996724: Long titles makes checkout button unclickable
#1975880: "Continue shopping" button unclickable.
#1875946: Confirmation Window Is Cut Off Vertically
It would be nice to get something like this committed.
Comment #8
benr commentedWorks for me.
Comment #9
silver157 commentedThanks
Comment #10
daniel wentsch commentedAwesome work deggertsen, thank you so much!
Comment #11
deggertsen commentedIt would be nice to have this added. I can add it myself if someone wants to make me a co-maintainer.
Comment #12
rv0 commentedAfter wasting an hour overriding css like crazy I can only conclude: YES. Please add this patch
Comment #13
rszrama commentedThanks for all the attention here. I've added deggertsen as a maintainer, but the one request I'd make is that we not introduce breaking changes to existing themes if at all possible. For example, the current patch here moves a class from a span to an anchor. Even if better HTML / CSS, if it breaks the myriad sites using this in production, it's a regression.
That said, sometimes changes are unavoidable. In those cases, we just need to make sure the release notes for the next release include careful instructions on changes that may impact custom styles / JavaScript. What's the impact been from this patch on live sites thus far?
Comment #14
deggertsen commented@rszrama, I understand what you mean; however, the current default HTML/CSS is quite broken IMHO. As noted in #7, this patch fixes a number of issues that people have run into with the current HTML/CSS. So far the feedback from others who have used this patch is positive with nothing negative other than what you pointed out.
Because this patch could potentially mess up custom overrides people have done in their own theme CSS I agree that patch notes would need to include careful instructions. I suggest that we apply the patch to dev for a period of time and then if there is no negative feedback here then we move it to rc3 with clear instructions.
Another potential option would be to create a new branch, but I don't really think this is a big enough change to merit that.
Comment #15
rszrama commentedCool, in that case, let's give it a shot and just ensure the release notes call out the HTML / CSS changes as worth special attention for folks who eventually update. : )
Comment #17
deggertsen commentedPatch committed to dev. Please review and comment here if there are any problems or successes.