Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moving to mixins #8

Closed
KittyGiraudel opened this issue Mar 16, 2014 · 3 comments
Closed

Moving to mixins #8

KittyGiraudel opened this issue Mar 16, 2014 · 3 comments

Comments

@KittyGiraudel
Copy link

While the point of this library is to move away from mixins to embrace the power of Sass placeholders in order to lighten CSS output, I feel like you should move back to a mixin powered code base.

Think about something like this:

@mixin hover-link($extend: true) {
  @if $extend {
    @extend %hover-link;
  }
  @else {
    text-decoration: underline;
    // And whatever you want
  }
}

%hover-link {
  @include hover-link($extend: false);
}

// Usage
.a {
  @include hover-link;
}

I can see a couple of interesting points to move to something like this for the entire codebase: first of all, it keeps the major pro of extending placeholders because it's basically what it does: extending placeholders.

But it also makes it possible for the user to use the features from a different @media scope simply by switching the only argument of the mixin to false. Then the mixin will behave as a regular mixin and dump the CSS rules without throwing a Sass error.

Also it keeps the Compass syntax of mixins, which may or may not be a deal breaker but I can see it as a positive change.

Finally, it gives you full control over what you want to do for the future of the lib. You can do whatever you want from a mixin (like generating and extending placeholders); you can't do that from within a placeholder.

@KittyGiraudel
Copy link
Author

Also, it would allow you to do some on-the-fly placeholder generation without asking anything from the user (like filling a list or something).

// Conf
$opacity-cache: ();

// Mixin
@mixin opacity($value) {
  @if not index($opacity-cache, $value) {
    $opacity-cache: append($opacity-cache, $value);
    @at-root %opacity-#{$value} {
      opacity: $value;
    }
  }
  @extend %opacity-#{$value};
}

// Usage
.a {
  @include opacity(0.5);
}

Edit: I suppose you could have a map called $cache at root level, with various keys like opacity, border-radius and stuff. This would keep things clean; no need to have dozens of cache variables.

@hagenburger
Copy link
Owner

Thanks for your comments.

In my projects I almost only use the non-numeric placeholders. Also I mostly use them, when the non-placeholder output would be just one line—so it’s much better to just add a selector somewhere else:

.my-class {
  color: red;
  display: block;

  .parent:hover & {
    @extend %display-none;
  }
}

So I see your suggestion as a big improvement over defining the placeholders upfront. I’m not sure about overriding Compass’ mixins. You would not notice the difference until you look at the output or in the Gemfile. The other thing is, there is no @super(...) for mixins in Sass to call the original Compass mixin within the placeholder selector.

So I would see two solutions:

  1. Define suffixed mixins (@include opacity-placeholder(0.5))—which gets quite long and ugly
  2. Write an own importer for Compass which renames the original mixins while importing

I would prefer № 2 as it results in more beautiful code in your projects.

This could be a reason to think about a version 2.0.0 as it’s a new API and would require Sass 3.3.

@KittyGiraudel
Copy link
Author

Or 3., namespace everything.

@include cp-opacity(0.5);
@include _opacity(0.5);
@include -opacity(0.5);
@include \:opacity(0.5);
@include \@opacity(0.5);
@include __opacity(0.5);

Or whatever you like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants