inuitcss / trumps.spacing-responsive

[DEPRECATED] [go to intuitcss/inuitcss]
https://github.com/inuitcss/inuitcss
12 stars 16 forks source link

This code is an embarrassment and needs DRYing out #1

Closed csswizardry closed 8 years ago

csswizardry commented 10 years ago

Way too much repetition in here. I don’t want to create some funky generate-everything-in-one-go kinda Sass, but I would like to try and wrap this up into a mixin. Unfortunately, progress so far hasn’t been great: http://sassmeister.com/gist/dfb20bc343bcd09e7011

nenadjelovac commented 10 years ago

@csswizardry this (quick)fixes it. Lines 98 and 106. That is currently the only way to access mixin variable from global scope in SASS. I think I remember there's an open issue in SASS repo, but I don't have the time to look into it now. BTW, there could be improvements to your mixins via SASS maps (do I sound like Hugo? :)), but as I said I don't have the time to offer suggestions now.

nenadjelovac commented 10 years ago

Ok, I couldn't help it and had to do maps version :) But while doing it I realized you might want to support older versions of sass and that's why you did it with maps. However, I suggest supporting SASS 3.3+ for inuit, but that's a whole other topic on its own.

KittyGiraudel commented 10 years ago

It turns out to be really easy to do as I first suspected. You need 3 configuration variables:

  1. $inuit-spacing, the default spacing used for all helpers
  2. $breakpoints, your breakpoints map
  3. $helpers, a fresh new map of helpers where keys are aliases and values properties
/**
 * Default inuit spacing
 * @type Number
 */
$inuit-spacing: 10px !default;

/**
 * Breakpoints map
 * @type Map
 */
$breakpoints: (
  "palm":          "screen and (max-width: 719px)",
  "lap":           "screen and (min-width: 720px) and (max-width: 1023px)",
  "lap-and-up":    "screen and (min-width: 720px)",
  "portable":      "screen and (max-width: 1023px)",
  "desk":          "screen and (min-width: 1024px)"
) !default;

/**
 * Helpers map
 * @access private
 * @type Map
 */
$helpers: (
  "m": "margin",
  "mt": "margin-top",
  "mr": "margin-right",
  "mb": "margin-bottom",
  "ml": "margin-left",
  "mh": "margin-right" "margin-left",
  "mv": "margin-top" "margin-bottom",

  "p": "padding",
  "pt": "padding-top",
  "pr": "padding-right",
  "pb": "padding-bottom",
  "pl": "padding-left",
  "ph": "padding-right" "padding-left",
  "pv": "padding-top" "padding-bottom"
) !default;

Then, our inuit-generate-responsive-variants. This mixin does one thing and one thing only: it loops over the breakpoints and include a inuit-generate-responsive-variant (no s this time) passing it the breakpoint name (or alias, as you wish).

/**
 * Generate responsive variants for all breakpoints
 * @requires $breakpoints
 * @requires {mixin} inuit-generate-responsive-variant
 */
@mixin inuit-generate-responsive-variants {
  @each $breakpoint-name, $breakpoint-value in $breakpoints {
    @include inuit-generate-responsive-variant($breakpoint-name);
  }
}

Last but not least, our inuit-generate-responsive-variant mixin. This is where most of the logic happens. Basically, we output the media query mapped to the given breakpoint name at root level. Then we loop over the helpers to build classes named after both the breakpoint name and the helper alias. And finally, we output the property with $inuit-spacing as value. Little condition has to be done here for mv/mh/pv/ph because they generate several properties instead of a single one.

/**
 * Generate responsive variants for given breakpoint
 * @param {String} $breakpoint-name
 * @requires $helpers
 * @requires $inuit-spacing
 */
@mixin inuit-generate-responsive-variant($breakpoint-name) {
  $breakpoint-value: map-get($breakpoints, $breakpoint-name);

  // Output a new media query at root level
  @at-root {
    @media #{$breakpoint-value} {

      // Loop over the helpers in $helpers
      @each $alias, $property in $helpers {

        // Create a new class
        .#{$breakpoint-name}-#{$alias} {

          // If helper contains several properties, loop over them 
          @if length($property) > 1 {
            @each $prop in $property {
              #{$prop}: $inuit-spacing !important;
            }
          }

          // If helper is a single property, output it
          @else {
            #{$property}: $inuit-spacing !important;
          }
        }
      }
    }
  }
}

And now we run the whole thing with a single line.

@include inuit-generate-responsive-variants;

If you need anything else, be sure to ask. By the way, you can play with the code here: http://sassmeister.com/gist/2675a636695ab23b403e.

KittyGiraudel commented 10 years ago

By the way, @nenadjelovac's version is surprisingly elegant as well. I can see pros and cons for both of them. If you need me to detail on this, feel free to ask.

nenadjelovac commented 10 years ago

By the way, @nenadjelovac's version is surprisingly elegant as well. I can see pros and cons for both of them.

@HugoGiraudel thank you Hugo. It could use better/smarter variable naming (eg $inuit-breakpoint-alias instead of $alias; error handling and stuff like that), but it's just a quick proof of concept.

And thank you for your version. It's awesome work (as usual, I'd have to add)! ;)

However, I am not sure if it fits into inuit framework, but Harry will have to have a say about that. The way I see it (@csswizardry correct me if I'm wrong), there should be global inuit mixin inuit-generate-responsive-variants() that loops through all breakpoints and that we can include whenever we need it. That mixin has to use @content to spit out its code.

Examples (mostly made up, but hit the point):


// _trumps.pull.scss
@include inuit-generate-responsive-variants() {
  @include inuit-setup-pull();  //sets up pull classes for all breakpoints, utilizing `$inuit-breakpoint-alias`
}

// _trumps.widths.scss
@include inuit-generate-responsive-variants() {
  @include inuit-setup-widths();  //sets up width classes for all breakpoints, utilizing `$inuit-breakpoint-alias`
}

// _trumps.hide.scss (made up partial)
@include inuit-generate-responsive-variants() {
  #{$inuit-breakpoint-alias}-hidden      { display: hidden !important; }
}

So I'm affraid having third map ($helpers) is not an option and/or brings complexity that inuit framework should do without.

Btw, I do realize you thought of this when you said that both solutions have its pros and cons, just thought I should elaborate for anyone reading, and to see what Harry has to say.

csswizardry commented 10 years ago

Hi guys,

Sorry for taking so long to weigh in on this.

@HugoGiraudel, thank you so much!

However…

I don’t want to create some funky generate-everything-in-one-go kinda Sass…

One of my key ‘requirements’ for new inuitcss is ‘as simple as possible, if not simpler’. Given that inuitcss is designed for use in teams (ergo, multiple developers) it is important that it looks as much like regular CSS as possible. Whilst Sass is incredibly powerful—and you have mastered it incredibly powerfully—this is not the style of code I wish to introduce into the project if I can help it (especially considering it only replicates functionality that currently already exists).

I feel incredibly ungrateful, and I apologise, but I don’t think anything that obscures behaviour should appear in the framework:

Using @nenadjelovac’s !global method seems the best approach; perhaps if I push where I took that to and we can see where to go from there…?

Thanks, all! H

KittyGiraudel commented 10 years ago

Hey, no problem at all Harry. Took me 5 minutes, it's no big deal.

@nenadjelovac's approach is better in the way that it uses @content. Yet to be perfectly honest I don't like much messing around with global variables right before and after @content. Always feel like a kind of hack to me.

But it's probably the best approach for your project.

Here is a slightly improved version of media-query mixin to handle errors:

@mixin media-query($mq) {
  @if not map-has-key($breakpoints, $mq) {
    @warn "Breakpoints map doesn't contain any key named `#{$mq}`.";
  }

  @else {
    @at-root {
      @media #{map-get($breakpoints, $mq)} {
        @content;
      }
    }
  }
}

Cheers. If you need anything, please ask.

lifeiscontent commented 9 years ago

@csswizardry why not introduce a $tmp var for dumping your globals in?

probably should rename it to something like $inuit-tmp

http://sassmeister.com/gist/lifeiscontent/4fbf3b3a2257dcfb5bba

csshugs commented 8 years ago

Since this repository is now officially deprecated, I close this.

We are doing it all a bit different in the new version anyway.

But thank you all for your time and your effort of finding a solution for this issue!