3
\$\begingroup\$

I'm trying out a new approach to SASS stylesheets, which I'm hoping will make them more organizined, maintainable, and readable.

I often feel like there is a thin line between code that is well structured and code that is entirely convoluted. I would appreciate any thoughts as to which side of the line this code falls.

I don't want to tell you too much more about what these styles are intended to produce -- my hope is that the code will explain this for itself. Also note that this is part of a larger project, so don't worry about missing dependencies, etc.

Questions for review

  1. How would you make this code easier to read/maintain?
  2. Can you understand what these styles are trying to produce?
  3. Is the purpose of the mixins/placeholders clear?

File structure:

theme/sass/partials/
    widget/
        collapsable/
            _appendicon.scss
            _closeall.scss
            _toggleswitch.scss
        collapsable.scss
        collapsablered.scss
    _button.scss

collapsable.scss

/**
 * Collapsable widget.
 *
 * The widget has "open" and "closed" states.
 * The widget has a Toggle Switch, which is visible in
 * both open and closed states.
 *    All other content is hidden in the closed state.
*/
@mixin setOpenState {
  &,
  &.state-open {
    @content;
  }
}

@mixin setClosedState {
  &.state-closed {
    @content;
  }
}

@mixin setToggleSwitchStyles {
  &>h1:first-child, .collapseableToggle {
    @content;
  }
}


@import "collapsable/closeall";
@import "collapsable/appendicon";
@import "collapsable/toggleswitch";


%collapsable {
  @include setOpenState {
    @include setToggleSwitchStyles {
      @extend %toggleSwitch;
    }
  }


  @include setClosedState {
    @extend %closeAllExceptToggle;

    @include setToggleSwitchStyles {
      @extend %toggleSwitchClosed;
    }
  }
}

collapsablered.scss

@import "collapsable";
@import "../button";

%collapsableRed {
  @extend %collapsable;

  @include setOpenState {
    @include setToggleSwitchStyles {
      @extend %buttonWithRedBg;
    }
  }

  @include setClosedState {
    @include setToggleSwitchStyles {
      @extend %buttonWithDarkBg;
    }
  }
}

collapsable/_closeall.scss

%closeAllChildren {
  * {
    display: none;
  }
}

%closeAllExceptToggle {
  @extend %closeAllChildren;

  @include setToggleSwitchStyles {
    display: block;

    .icon-sprite {
      display: inline-block;
    }
  }
}

collapsable/_appendicon.scss

@import "compass/utilities/general/clearfix";
@import "../../icon";

@mixin appendIcon {
  @include pie-clearfix;

  .icon-sprite {
    margin-right: 5px;
    vertical-align: -3px;
  }

  &:after {
    content: '';

    position: relative;
    top: 2px;

    float: right;

    @content;
  }
}

%withCloseIcon {
  @include appendIcon {
    @extend .icon-close;   // defined in _icon.scss
  }
}

%withOpenIcon {
  @include appendIcon {
    @extend .icon-rChevronDk;   // defined in _icon.scss
    top: 1px;
  }
}

collapsable/_toggleswitch.scss

%toggleSwitch {
  cursor: pointer;
  @extend %withCloseIcon;
}


%toggleSwitchClosed {
  @extend %toggleSwitch;
  @extend %withOpenIcon;
}

partials/_button.scss

@import "typography";

%buttonWithRedBg {
  @extend %textOnRedBg;  // defined in _typography.scss

  cursor: pointer;

  &:hover {
    background-color: $redDk;
  }

  &:active {
    background-color: $black;
  }
}

%buttonWithDarkBg {
  @extend %textOnDarkBg;  // defined in _typography.scss

  cursor: pointer;

  &:hover {
    background-color: #000;
  }

  &:active {
    background-color: $redDk;
  }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ There appears to be a CSS error in your icon-sprite class. You have vertical-align: -3px, but the vertical-align property only takes specific values like top/bottom/middle/text-top/etc. Also, is this code intended to be portable (Compass extension)? \$\endgroup\$ – cimmanon Dec 4 '13 at 16:12
  • \$\begingroup\$ As I said, this is a component of a larger project, not a portable extension -- though I am concerned with modularity and reusability. Also, I'm looking for comments on readability/structure/etc., not syntax errors -- though vertical-align does take px arguments, for the record. \$\endgroup\$ – edan Dec 5 '13 at 3:26
4
\$\begingroup\$

Overall, your naming conventions are pretty good. I don't feel like I need to go look at mixins themselves to figure out what their purpose is. The extensive use of extends does concern me, since it can lead to larger CSS rather than smaller like you might expect (see: Mixin, @extend or (silent) class?).

Your %textOnDarkBg and %textOnRedBg extend classes might be redundant. If you're not already using Compass, you might want to take a look at it. It offers a function as well as a mixin for setting a good contrasting color against your desired background color (see: http://compass-style.org/reference/compass/utilities/color/contrast/). Highly useful if your project is intended to be themed.

Generally speaking, using colors for class names isn't very clear unless the content is about color (eg. a color wheel or a rainbow). What is red for? Is it for errors? Or maybe a call to action? The same thing goes for dark. Using inverted or closed might be better choices. If the site's design is already dark, a dark button probably doesn't make much sense.

Your code only allows the user to have exactly 2 colors (default and red), which seems more limited than it needs to be. You could easily make it very flexible by making use of lists (or maps, which will be in the next version of Sass). Here's an example from my own project:

//      name     dark    light
$dialog-help:    #2E3192 #B9C2E1 !default; // purple
$dialog-info:    #005FB4 #BDE5F8 !default; // blue
$dialog-success: #6F7D03 #DFE5B0 !default; // green
$dialog-warning: #A0410D #EFBBA0 !default; // orange
$dialog-error:   #C41616 #F8AAAA !default; // red

$dialog-attributes:
    ( help    nth($dialog-help, 1) nth($dialog-help, 2)
    , info    nth($dialog-info, 1) nth($dialog-info, 2)
    , success nth($dialog-success, 1) nth($dialog-success, 2)
    , warning nth($dialog-warning, 1) nth($dialog-warning, 2)
    , error   nth($dialog-error, 1) nth($dialog-error, 2)
    ) !default;

@each $a in $dialog-attributes {
    $name: nth($a, 1);
    $color: nth($a, 2);
    $bg: nth($a, 3);

    %dialog-colors.#{$name} {
        color: $color;
        background-color: $bg;
    }

    %dialog-colors-inverted.#{$name} {
        color: $bg;
        background-color: $color;
    }

    %badge-colors.#{$name} {
        background-color: $color;
        color: $background-color;
    }

    %button-colors.#{$name} {
        @include button($base: $bg) {
            @include button-text($color, inset);

            @include button-states;
        }
    }

    %button-colors-inverted.#{$name} {
        @include button($base: $color) {
            @include button-text($bg, inset);

            @include button-states;
        }
    }

    %button-colors-faded.#{$name} {
        @include button($base: fade($bg, 10%)) {
            color: #CCC;

            @include button-states;
        }
    }
}

In case you're wondering why I'm using multiple classes, I've setup a short demo: http://sassmeister.com/gist/7792677

\$\endgroup\$
  • \$\begingroup\$ I'm glad the structure and use of the mixins is easy to grok. The use of @extend vs @mixin is still a bit of a mystery to me, though I've been trying to use @extend to denote a type of style (noun), and @include to do something to a style (verb). I think you're right about the color naming. I split out collapsableRed because I wanted to make it easy to extend the base collapsable widget for different themes. It would probably be better to use a named theme instead of a color name (eg myAwesomeThemeCollapsable). I wouldn't use .42pxWideHeader instead of .sidebarHeader \$\endgroup\$ – edan Dec 5 '13 at 19:18
  • \$\begingroup\$ And yes, I am using Compass. The %textOnWhateverColorBg runs into the same problem as %collapsableWhateverColor -- I shouldn't be naming a class with a specific style value. I'll have to think of a better name. Maybe %textColorsActive and %textColorsInactive would be more descriptive. \$\endgroup\$ – edan Dec 5 '13 at 19:23

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.