Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.From a developer point of view, working with Commerce is really nice, but from a themer's point of view working with Commerce can be a bit of a pain some times.
Why is that you my ask?
Use the BAT style notation, for detailed info see mortendk's blog post. To keep it short, Drupal 7 has in some extent and Drupal 8 will most likely fully using the BAT css file notations.
Base (This css should be included, if you remove it things will most likely break.)
Admin (This is for admin goodness)
Theme (For all the colors etc that many themers want to rip out ASAP.
CSS file names should be module_name.BAT.css
Doing this would be a huge improvement.
The second things that bugs me when theming commerce sites, is that it's very easy to see that all the styles and many theming functions as well have been created with the purpose of looking good in the default Bartik theme, not to create semantic HTML or sensible styles. In comparison to Ubercart commerce is a huge improvement, but we can do so much better.
One example of this is #1098028: Improve the markup of the checkout review pane., it seems that the sole reason for not improving the markup making it easier to theme and more semantic is a comparison with how it looks on Bartik. How many live Drupal sites out there do we have with Bartik? How many do we have using a different theme? I don't now the numbers but I'm guessing we have about 100/1000 times more sites using non Bartik themes. So why make it hard on themers working with Commerce, just to get the demos on bartik a bit prettier? In my mind, it makes absolutely no sense at all.
Another example, which is what triggered this is that commerce has some styles for the checkout links:
.checkout-buttons .checkout-cancel,
.checkout-buttons .checkout-back {
border: 0;
background: none;
color: #0071B3;
padding: 4px 6px;
}
.checkout-buttons .checkout-cancel:focus,
.checkout-buttons .checkout-back:focus,
.checkout-buttons .checkout-cancel:hover,
.checkout-buttons .checkout-back:hover {
background: none;
color: #018fe2;
text-decoration: underline;
}
It's fine to create some position styles, if it's generic, which seems to be the case, but why does commerce create styles for the links next to the submit buttons in the checkout flow that
- Determines color
- Determines background
- Determines underline state
It would be nice to be able to create a default rule in your style sheet that determines how you want your links to look like and behave, but with commerce, you need to create a lot of these overriding rules for your default to take effect all over your site.
These are just examples. In itself it's not a huge deal to have to override the styles for these links. But why make these obstacles for themers. These little things it what makes the different between a good and a great commerce system. Right now I believe commerce is the best commerce solution for Drupal and one of the best out there. But Commerce should remain the best, these are the kind of things that needs attention. In the issue above I asked for input from Ryan 13 weeks ago, the issue itself is 40 weeks old. It's disappointing to see no will to improve Commerce in this area especially when some one is ready to deliver the work required.
This turned out to be a bit of a rant, I hope you understand the motivation behind this is a wish for Commerce to improve. It's amazing to see what's possible to do with Commerce, but I think we can do better, and make this a success removing these pain points during developing.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | 1386816-5.patch | 39.69 KB | amateescu |
| #4 | 1386816.patch | 39.67 KB | amateescu |
Comments
Comment #1
googletorp CreditAttribution: googletorp commentedI just noticed the styles mentioned isn't used for links but to make submit buttons look like links, which makes sense.
The point about keeping it in a .theme.css file still holds true, and that a lot of the fronted styling is focused on making bartik look good rather than providing sensible default styles.
Comment #2
googletorp CreditAttribution: googletorp commentedI've created a patch for this and committed it to my sandbox of commerce: 8f772fb
That patch looks pretty massive since all the css files have been renamed, but the change isn't that big. I've tried to merge css files as much as possible instead of having a lot of tiny CSS files (1-5 rules) We now have either base admin or theme styles.
For consistency all ui modules uses it's parent's admin.css file (this makes sense for all modules except one, where the parent module didn't have any css files. But I figured it would make more sense to create a non ui.admin.css files for constancy.
The net result is a lot druapl_add_css a few less css files and a lot of happy themers.
Comment #3
googletorp CreditAttribution: googletorp commentedI had unsaved edits in my editor, so remade the patch to include those:
0b669c6
Comment #4
amateescu CreditAttribution: amateescu commentedWhoa, this was a big change to review, but a very good one :)
Attaching a patch with a couple of changes:
- cleaned up all the @file and inline doxygen blocks so they conform to the coding standards
- this piece of code was everywhere in various forms, 4 files were created just for it
so I created a commerce.admin.css file and included it in commerce_ui.info.
Looks RTBC to me.
Comment #5
amateescu CreditAttribution: amateescu commentedHmm, the selector needs to be more specific if we're including it on all pages.
Comment #6
rszrama CreditAttribution: rszrama commentedIt took me a little while to realize what you've done by adding
stylesheets[all][] = theme/commerce.admin.cssto commerce.info, but this feels pretty heavy-handed to me. If I just view the source of the front page of the site, I'll see a commerce.admin.css stylesheet in the head of the HTML. I think I'd just prefer to add this in in the places where it's required, though I agree with the more specific selector (hmm, which may not be necessary if we're only explicitly including this stylesheet when necessary) and unified CSS file. I'll sleep on it.Other than that, I'd argue that the first two styles in commerce_price.theme.css actually belong in commerce_price.base.css. These are the styles that place the price amount textfield and currency code select lists on the same line in forms including the price widget. The primary use case is the back-end, but it's entirely possible for price widgets to appear on the front-end (think a custom price field on the product line item type exposed to the Add to Cart form). The same goes for the styles in commerce_tax.theme.css pertaining to the select list for entering prices including VAT, though I suppose here it's arguable whether they should be in a .base.css or .admin.css.
Everything else looks fine and passes a visual inspection. Great job on the patch, and congratulations on resisting the urge to fix the actual markup behind some of these styling decisions at the same time. ; )
Once the items above are either retorted or fixed, we'll get this in!
Comment #7
googletorp CreditAttribution: googletorp commentedOn iPhone so krepning it short.
I don't think we should merge CSS files in this patch. If we wanted to create an commerce.cas file we should add all admin. CSS in it.
The rule of thumb for base styles is that things break when removed. Typical Ajax stuff. Visual improvements go in theme. The placement of price components is the latter. So I believe we should go for my patch and commit it :-)
Comment #8
rszrama CreditAttribution: rszrama commentedThe ones I said to move don't deal with the placement of price components but the logical arrangement of form elements, such that the currency code select list (which doesn't have a label of its own) will appear beside the amount textfield. Same for the horizontal alignment of the tax element. Does that clarify?
Comment #9
rszrama CreditAttribution: rszrama commentedWait, wait. I see what you're saying. Yes, let's leave them in the .theme.css. It made sense when I looked at the only item you'd included in a .base.css. Visually, getting rid of the inlines / floats for price fields and tax inclusion float will make it look bad, but all the elements would still in fact be visible - just aligned vertically. So even if no one ever overrides those styles, they're fine in the .theme.css.
I reverted the change to a single commerce.admin.css but kept the updates to various comments. No worries, though - commit credit is still going to googletorp. ; )
Commit: http://drupalcode.org/project/commerce.git/commitdiff/f8b34d6
Comment #10
googletorp CreditAttribution: googletorp commentedThanks Ryan, was a bit busy at the time, (was way early and needed to do some non Drupal stuff), but glad we got it committed.