current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 12 down vote favorite

I've leaped into SASS and am loving it. I'm asking for comments here on whether my SASS is well-formatted.

Here's the original CSS based on Zurb's Foundation Pagination styles, but adapted for use with Laravel's built-in pagination: 

div.pagination ul { display: block; height: 24px; margin-left: -5px; }
div.pagination ul li { float: left; display: block; height: 24px; color: #999; font-size: 14px; margin-left: 5px; }
div.pagination ul li a { display: block; padding: 1px 7px 1px; color: #555; font-weight: normal}
div.pagination ul li:hover a,
div.pagination li a:focus { background: $bgltblue; }
div.pagination ul li.disabled a { cursor: default; color: #999; }
div.pagination ul li.disabled:hover a,
div.pagination li.disabled a:focus { background: transparent; }
div.pagination ul li.active a { background: $headblue; color: white; font-weight: normal; cursor: default; }
div.pagination ul li.active a:hover,
div.pagination li.active a:focus { background: $headblue; }

Here's my manual conversion to SASS:

div.pagination {
  ul {
    display: block; height: 24px; margin-left: -5px;
    li {
      float: left; display: block; height: 24px; color: #999; font-size: 14px; margin-left: 5px;
      a {
        display: block; padding: 1px 7px 1px; color: #555; font-weight: normal;
      }
      &:hover a {
        background: $bgltblue;
      }
      &.disabled {
        a {
          cursor: default; color: #999;
        }
        &:hover a {
          background: transparent;
        }
      }
      &.active {
        a {
          background: $headblue; color: white; font-weight: normal; cursor: default;
          &:hover {
            background: $headblue;
          }
        }
      }
    }
  }
  li {
    a:focus {
      background: $bgltblue;
    }
    &.disabled {
      a:focus {
        background: transparent;
      }
    }
    &.active {
      a:focus {
        background: $headblue;
      }
    }
  }
}

I'd like to know if you think I have reduced it correctly, and where I might make improvements. I plan to replace the colours with vars already.

share|improve this question
    
Doesn't Foundation already use Sass? –  cimmanon Sep 5 '13 at 20:33

1 Answer 1

up vote 12 down vote accepted

Some comments on your style of selecting:

  • div.pagination is overqualified. A container is almost always going to be a div, so you just can write .pagination
  • In many cases, there is no use for an extra container. You might as well apply the pagination class to the list itself: <ul class="pagination">

  • You also have a lot of redundancy in your selectors because you always use ul and li in the chains. This is not necessary and could be made easier:

    .pagination ul {}

/* li's can only appear in lists, no need to add it to the selector */
.pagination li {}

/* The only links inside your pagination are usually in relation to the pagination */
.pagination a {}

  • If you have lists inside your pagination that need separate styling, use separate classes or place them outside of the actual pagination

  • Don't overqualify the .active and .disabled classes as well. They will end up being on the list items anyway

Conclusion:

You nest far too deep. Avoid nesting selectors deeper than 2–3 levels. Otherwise you get bloated CSS like this.

Compare my SCSS to yours:

.pagination {
    display: block;
    height: 24px;
    margin-left: -5px;

    li {
        float: left;
        display: block;
        height: 24px;
        color: #999;
        font-size: 14px;
        margin-left: 5px;

        &:hover a {
            background: $bgltblue;
        }

        &.disabled {
            a {
                cursor: default;
                color: #999;

                &:hover,
                &:focus {
                    background: transparent;
                }
            }
        }

        &.active {
            a {
                background: $headblue;
                color: white;
                font-weight: normal;
                cursor: default;

                &:hover,
                &:focus {
                    background: $headblue;
                }
            }
        }
    }

    a {
        display: block;
        padding: 1px 7px 1px;
        color: #555;
        font-weight: normal;

        &:focus {
            background: $bgltblue;
        }
    }
}
share|improve this answer
1  
Really nice explanation, thanks! –  Mere Development Jan 27 '14 at 18:02

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.