Problem/Motivation

Was looking through the CSS and would like to suggest some tweaks.

Proposed resolution

  1. Remove browser prefixes, they aren't needed and can be a pain to maintain.
    http://caniuse.com/#feat=css-boxshadow
    http://caniuse.com/#search=border-radius
  2. 0px is shorter and saves a few characters as 0.
  3. display: inline + margin doesn't work. Change to inline-block if you want to effect margin.
  4. Group the properties together that are related (position, box model, text, styles, etc)

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#12 css_update-2413317-12.patch3.29 KBjoelpittet
#9 css_update-2413317-9.patch3.86 KBjoelpittet
#9 interdiff.txt2.49 KBjoelpittet
#6 interdiff.txt494 bytesjoelpittet
#6 css_update-2413317-6.patch2.18 KBjoelpittet
#5 css_update-2413317-5.patch2.18 KBjoelpittet
#1 css_update-2413317-1.patch2.07 KBjoelpittet

Comments

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Active » Needs review
StatusFileSize
new css_update-2413317-1.patch2.07 KB
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Should be no visual difference with that.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Needs work 
+++ b/css/dc_ajax_add_cart.css
@@ -1,38 +1,33 @@
-a.add-to-cart-close {
...
+.add-to-cart-close {

I see why you are being explicit here, likely should use a data attribute for the JavaScript interaction and make the two classes unique to the items they are styling.

Also, there are a couple of background: that could be much simpler just using background-color: properties.

subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented

Thank you very much for the suggestions joelpittet++
I was testing the css code with http://csslint.net/ and I found other warnings beside your proposed resolution. Could you please update the css code according to your suggestions and tool's suggestions. And I think using a unique class for the close (a.add-to-cart-close) will be the best approach here, whats your opinion? Waiting for the patch :)

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Assigned: Unassigned » joelpittet
Status: Needs work » Needs review
StatusFileSize
new css_update-2413317-5.patch2.18 KB

@subhojit777 the csslint items are for the adjoining classes for IE6? Unless you had some other settings set in it, otherwise I wouldn't worry about it. IE8 is on the chopping block.

I'd avoid removing or changing CSS classes on a full release because likely someone has already forked your CSS/templates and we should try not to break those sites. Which is always the no-fun part of maintaining stuff:P

So I'll leave the class name refactoring out of this so we don't break peoples CSS intentionally. And hopefully we don't unintentionally either;)

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Assigned: joelpittet » Unassigned
StatusFileSize
new css_update-2413317-6.patch2.18 KB
new interdiff.txt494 bytes

Whoops, missed a semicolon, classic:P

subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented
Status: Needs review » Needs work

No I did not had any other settings in csslint.

+++ b/css/dc_ajax_add_cart.css
@@ -1,38 +1,33 @@
-  -moz-box-shadow: 0 1px 5px #333;
-  -moz-border-radius: 20px;

You have not included these styles.

Regarding the css refactoring thing, I understand your concern that it might break existing sites. Actually I have did something similar here #2365599: Pass cart render array to template. , I have changed the templates rendering shopping cart block. So it will break existing sites if they update it. But this change is important, and it cannot be avoided. Any suggestions on what would be the best thing to do in this situation? Can I release this as 2.x release, and write about this in module page. Thanks :)

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Oh I thought the real properties where there already, I guess not.

I'd suggest using a data-attribute for the JS to interact with instead of a class. And then two classes for the elements.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt2.49 KB
new css_update-2413317-9.patch3.86 KB

Something like this.

subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented
Status: Needs review » Needs work 
  1. +++ b/css/dc_ajax_add_cart.css
@@ -1,38 +1,35 @@
-  background-color: #fff;
-  border: 2px solid #ccc;
-  color: #7f7f7f;
-  -khtml-border-radius: 20px;
-  left: 30%;
-  padding: 20px;
   position: fixed;
-  -moz-box-shadow: 0 1px 5px #333;
-  -moz-border-radius: 20px;
-  right: 30%;
-  top: 100px;
-  -webkit-box-shadow: 0 1px 5px #333;
-  -webkit-border-radius: 20px;
   z-index: 101;
+  top: 100px;
+  right: 30%;
+  left: 30%;
+  padding: 20px;
+  color: #7f7f7f;
+  border: 2px solid #ccc;
+  border-radius: 20px;
+  background-color: #fff;
+  box-shadow: 0 1px 5px #333;

    top, right, left, background-color have unrelated changes. Only the position of the styles have changed. Please undo them. 

  2. +++ b/css/dc_ajax_add_cart.css
@@ -1,38 +1,35 @@
-  cursor: pointer;
+.add-to-cart-close {
   float: right;
+  width: 26px;
   height: 26px;
-  margin-right: -30px;
   margin-top: -30px;
-  width: 26px;
+  margin-right: -30px;
+  cursor: pointer;

    cursor, margin-right, width are unrelated changes. 

  3. +++ b/css/dc_ajax_add_cart.css
@@ -45,28 +42,25 @@ a.add-to-cart-close {
-  cursor: pointer;
   margin-bottom: 20px;
   padding: 5px 10px;
+  cursor: pointer;

    Unrelated changes. 

  4. +++ b/css/dc_ajax_add_cart.css
@@ -45,28 +42,25 @@ a.add-to-cart-close {
-  color: #626262;
+  background-color: #969696;
   font-weight: bold;
+  color: #fff;

    Why are we chaging the color here. 

  5. +++ b/css/dc_ajax_add_cart.css
@@ -45,28 +42,25 @@ a.add-to-cart-close {
-  color: #ffffff;
+.option-button.checkout a {
   font-weight: bold;
+  color: #626262;

    Why are we changing the color. 

  6. +++ b/css/dc_ajax_add_cart.css
@@ -107,7 +101,7 @@ a.add-to-cart-close {
-  clear: both;
   float: left;
+  clear: both;

    Please undo this.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

I changed the order of properties because I believe my linter did that to have like properties grouped together and ordered correctly. TBLR for example.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review
StatusFileSize
new css_update-2413317-12.patch3.29 KB

The color is just moved from the span tag to the a tag so that the styling of those two buttons would be consistently applied to the same type of element.

Here's most of the moves removed so it's easier to review.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Bump

subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented
Status: Needs review » Fixed

Committed thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.