2
\$\begingroup\$

I figured this morning that I wanted to learn how to use SASS, so I decided to rewrite an existing project's CSS using SASS. The project itself isn't complete so this is just a rough draft of the final code.

What I'm looking for here is to know what I'm doing well and what I'm doing wrong. If I know what I'm doing well I can continue doing that and if I know what I'm doing wrong I know what to change.

Edit: A general concern of mine is my naming of variables and mixins. It feels like I should name them something else but I really can't seem to come up with something better. Any suggestions on this matter?

  • How can I improve the structure of the code?
  • What should I remove from the code?
  • What should I add to the code?
  • Optimization tips?
  • Anything else worth pointing out

//Colors
$white: white;
$lightgrey: lightgrey;

//Common margin and padding values
$margin0: 0;
$margin5: 5px;
$margin10: 10px;
$margin15: 15px;
$margin20: 20px;

$padding0: 0;
$padding5: 5px;
$padding10: 10px;
$padding15: 15px;
$padding20: 20px;

//Font styles
$fontSize1: 0.9em;
$fontSize2: 1em;
$fontSize3: 1.2em;
$fontSize4: 1.4em;
$fontSize6: 1.6em;
$fontFamily: 'Helvetica Neue', Helvetica, Arial, sans-serif;

//Widths
$width100: 100%;
$width80: 80%;
$width60: 60%;
$width40: 40%;
$width20: 20%;

//Random values
//..

//Border thickness
$thin: 1px;
$medium: 3px;
$thick: 5px;

//Border types
$solid: solid;
$dotted: dotted;
$dashed: dashed;

%margins {
    margin: 0;
    padding: 0;
}

@mixin basicProps($width: null, $height: null, $bg: null, $margins: null) {
    @if $margins != null { @extend %margins; }
    width: $width;
    height: $height; 
    background: $bg; 
}

@mixin vertAlign($height, $lineHeight: $height) {
    height: $height;
    line-height: $lineHeight;
}

@mixin aligns($top: null, $right: null, $bottom: null, $left: null) {
    top: $top; 
    right: $right; 
    bottom: $bottom; 
    left: $left; 
}

@mixin fullBorder($thickness: null, $type: null, $color: null, $radius: null) {
    @if $radius != null { border-radius: $radius; }
    border: $thickness $type $color;
}

@mixin singleBorder($thickness, $type, $color, $top: null, $right: null, $bottom: null, $left: null) {
    @if $top != null { border-top: $thickness $type $color; }
    @if $right != null { border-right: $thickness $type $color; }
    @if $bottom != null { border-bottom: $thickness $type $color; }
    @if $left != null { border-left: $thickness $type $color; }
}

@mixin padding($value: 0, $top: 0, $right: 0, $bottom: 0, $left: 0, $omit: null) {
    @if $top == 0 and $right == 0 and $bottom == 0 and $left == 0 { padding: $value; }
    @else if $omit != null { padding: $top $omit }
    @else if $top != 0 or $right != 0 or $bottom != 0 or $left != 0 { padding: $top $right $bottom $left; }
}

@mixin margin($value: 0, $top: 0, $right: 0, $bottom: 0, $left: 0, $omit: null) {
    @if $top == 0 and $right == 0 and $bottom == 0 and $left == 0 { margin: $value; }
    @else if $omit != null { margin: $top $omit }
    @else if $top != 0 or $right != 0 or $bottom != 0 or $left != 0 { margin: $top $right $bottom $left; }
}

body {
    @include basicProps($width: $width100, $bg: $white, $margins: true); 
    font-family: $fontFamily;
}

#container {
    @include basicProps($width: $width40);
    @include fullBorder($thickness: $thin, $type: $solid, $color: $lightgrey);
    @include margin($top: 100px, $omit: auto);
    position: relative;
}

#info-container {
    @include basicProps($width: $width100);
    @include vertAlign(70px);
    @include singleBorder($bottom: true, $thickness: $thin, $type: $solid, $color: $lightgrey);
    float: left;
    position: relative;

    form {
        @include basicProps($width: 235px, $height: 30px);
        @include margin($left: $margin20);
    }

    input:nth-child(1) {
        @include basicProps($width: inherit, $height: inherit);
        @include padding($left: $padding15);
    }

    button {
        @include vertAlign(24px);
        @include basicProps($width: 24px, $bg: none);
        @include aligns($bottom: 5px);
        border: 0;
        outline: 0;
        font-weight: bold;
        position: absolute;
    }

    button:nth-child(2) {
        @include aligns($right: 10px);
        background: url(img/add.png) no-repeat;
        background-size: cover;
    }

    button:nth-child(3) {
        @include aligns($right: 45px);
        background: url(img/myfav.png) no-repeat;
        background-size: cover;
    }
}

#search-hits {
    @include basicProps($width: 250px, $bg: $white);
    @include fullBorder($thickness: $thin, $type: $solid, $color: $lightgrey);
    @include margin($top: -18px);
    max-height: 150px;
    position: absolute;
    display: none;
    overflow: auto;

    ul {
        @extend %margins;    
    }

    li {
        @include vertAlign($height: 30px);
        @include margin($value: 0);
        @include padding($left: $padding15);
    }
}

#list-container {
    @include basicProps($width: 45%);
    @include singleBorder($thickness: $thin, $type: $solid, $color: $lightgrey, $right: true);
    @include padding($bottom: $padding15);
    @include margin($top: 70px);
}

#add-form-container {
    @include basicProps($width: 55%, $height: 200px);
    @include aligns($top: 70px, $right: 0);
    @include singleBorder($thickness: $thin, $type: $solid, $color: $lightgrey, $bottom: true);
    float: right;
    position: absolute;
    display: none;

    form {
        @include basicProps($width: 90%);
        @include margin($top: 20px, $omit: auto);
    }

    tr td {
        @include padding($bottom: $padding5);
    }

    tr td:nth-child(1) {
        @include padding($right: $padding5);
        text-align: right;
    }

    tr td:nth-child(3) {
        @include basicProps($width: 90px);
    }

    tr:nth-child(5) td:nth-child(2) {
        text-align: center;    
    }

    span {
        @include margin($left: $margin5);
        font-size: $fontSize1;
    }
}

ul {
    @include margin($value: $margin0);
    @include padding($top: $padding15);
    list-style: none;

    li {
        @include singleBorder($thickness: $thin, $type: $solid, $color: $lightgrey, $bottom: true);
        @include margin($bottom: $margin10, $left: $margin20);

        li {
            cursor: pointer;
        }
    }
}

input {
    outline: 0;
}
\$\endgroup\$
4
\$\begingroup\$

For me, a lot of this looks like you're trying to... Sassify it too much.

Let's start by looking at your padding mixin:

@mixin padding($value: 0, $top: 0, $right: 0, $bottom: 0, $left: 0, $omit: null) {
    @if $top == 0 and $right == 0 and $bottom == 0 and $left == 0 { padding: $value; }
    @else if $omit != null { padding: $top $omit }
    @else if $top != 0 or $right != 0 or $bottom != 0 or $left != 0 { padding: $top $right $bottom $left; }
}

This looks way more cumbersome than just using the padding shorthand:

padding: 1em 0;

Contrast that with your verticalAlign mixin:

@mixin vertAlign($height, $lineHeight: $height) {
    height: $height;
    line-height: $lineHeight;
}

This is exactly the sort of thing you want to use a mixin for. It has a semantic name so you know exactly what those styles are for. It's not just about assigning one property to one value.

As a general rule of thumb, mixins are best suited for these types of tasks:

  • Perform calculations on the arguments
  • Reuse the arguments within the mixin
  • Provide styles in addition to styles based on the arguments
  • Encapsulating a common design pattern under a semantic name

If you're not doing any of these things, then you probably don't want a mixin.

Naming Conventions

If you're new to programming, coming up with good names for variables can be tough. One thing you want to keep in mind is that the name should make sense even if the value changes. So variables like these have poor names:

$margin0: 0;
$margin5: 5px;
$margin10: 10px;
$margin15: 15px;
$margin20: 20px;

Instead, variables like these might make more sense:

$spacing-narrow: 5px;
$spacing: 10px;
$spacing-wide: 15px;
$spacing-extra-wide: 20px;

Then again, it might make more sense to just have one variable and then do the math:

$spacing: 10px;

.foo {
    margin: $spacing ($spacing * 2);
}

Design Patterns

One thing that might help is if you start thinking about the things you repeat, then write code that encapsulates those little blocks. For instance, you might have 3 basic border styles that are part of your design:

$primary-neutral: #999;
$secondary-neutral: #CCC;
$tertiary-neutral: #EEE;

$primary-border: 2px solid $primary-neutral;
$secondary-border: 1px solid $secondary-neutral;
$tertiary-border: 1px dotted $secondary-neutral;

Now they're easy to incorporate into your design:

h1 {
    border: {
        top: $primary-border;
        bottom: $primary-border;
    }
    color: $tertiary-neutral;
}

.sidebar {
    border-right: $secondary-border;
}

Redundancy

The @if statement here is unnecessary:

@mixin fullBorder($thickness: null, $type: null, $color: null, $radius: null) {
    @if $radius != null { border-radius: $radius; }
    border: $thickness $type $color;
}

If a variable is null, the property associated with it won't be output. This mixin will give the same result:

@mixin fullBorder($thickness: null, $type: null, $color: null, $radius: null) {
    border-radius: $radius;
    border: $thickness $type $color;
}

CSS Niggles

If you're absolutely positioning an element, floats do nothing.

#add-form-container {
    float: right;
    position: absolute;
}

If you're able to ignore IE8, you might like this vertical centering technique better:

@mixin vertical-center {
    position: relative;
    top: 50%;
    transform: translateY(-50%);
}

http://davidwalsh.name/css-vertical-center

Outline on form controls is considered an accessibility feature. You should only be removing it if you're replacing it with something else that fits in better with your design:

input {
    outline: 0;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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