Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Merged omega and nth-omega #34

Closed
wants to merge 3 commits into from

3 participants

@dukex

This pull request is about #32 conversation,

I think a lot about backward compatibility, I created a small project to test the css property of element, can see here(https://github.com/dukex/neat_test_example)

Now, it:

// old omega
.omega-default{  @include omega; }
.omega-table{  @include omega(table); }
.omega-default-left{  @include omega(block, left); }
.omega-table-left{  @include omega(table, left); }

// old nth-omega merged
.omega-nth-default{ @include omega(4n) }
.omega-nth-table{  @include omega(4n, table); }
.omega-nth-default-left{  @include omega(4n, block, left); }
.omega-nth-table-left{  @include omega(4n, table, left); }

Will be compiled to it:

.omega-default { margin-right: 0; }
.omega-table { padding-right: 0; }
.omega-default-left { margin-left: 0; }
.omega-table-left { padding-left: 0; }

.omega-nth-default:nth-child(4n) { margin-right: 0; }
.omega-nth-table:nth-child(4n) { padding-right: 0; }
.omega-nth-default-left:nth-child(4n) {  margin-left: 0; }
.omega-nth-table-left:nth-child(4n) { padding-left: 0; }
@treppo

You got a typo there, should be:

@warn "The nth-omega() mixin was deprecated. Please use omega()";
@dukex

@treppo Thx, I fixed it!

@kaishin kaishin commented on the diff
app/assets/stylesheets/grid/_grid.scss
@@ -89,28 +89,25 @@ $fg-max-width: $max-width;
}
// Remove element gutter
-@mixin omega($display: block, $direction: right) {
- @if $display == table {
- padding-#{$direction}: 0;
- }
-
- @else {
- margin-#{$direction}: 0;
+@mixin omega($display_or_nth: block, $direction_or_display: right, $direction: right) {
@kaishin Admin
kaishin added a note

That's an interesting solution, however I'm not sure about using one argument for two different things. In cases such as this one, I tend to favor the flexibility of list arguments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kaishin
Admin

I have already taken a stab at merging the two mixins using a list argument instead:

@mixin omega($query: block, $direction: right) {
  $display: if(belongs-to(table, $query), table, block);

  @if length($query) == 1 {
    // TODO: refactor as private function
    @if $query == table or $query == block or $query == inline-block or $query == inline {
      @if $display == table {
        padding-#{$direction}: 0;
      }

      @else {
        margin-#{$direction}: 0;
      }
    }

    @else {
      &:nth-child(#{$query}) {
        margin-#{$direction}: 0;
      }
    }
  }

  @else if length($query) == 2 {
    @if $display == table {
      &:nth-child(#{nth($query, 1)}) {
        padding-#{$direction}: 0;
      }
    }

    @else {
      &:nth-child(#{nth($query, 1)}) {
        margin-#{$direction}: 0;
      }
    }
  }

  @else {
    @warn "Too many arguments passed to the omega() mixin."
  }
}

I like @dukex's 'polymorphic' approach, but I think it's a bit confusing. Thoughts?

@dukex

Ya the 'polymorphic' approach ins't beautiful approach, I like your approach and I think is important the mixin accept two different argument

@kaishin
Admin

@dukex You did once write some tests for Neat, do you still have those somewhere? I'm thinking of using them for testing.

@dukex

yes, I create a repo, it's very simple only a Rakefile, on app/assets/stylesheets/ folder should be bourbon installed

https://github.com/dukex/neat_test_example

@dukex

I tested it

// old omega
.omega-default{  @include omega; }
.omega-table{  @include omega(table); }
.omega-default-left{  @include omega(block, left); }
.omega-table-left{  @include omega(table, left); }

// old nth-omega merged
.omega-nth-default{ @include omega(4n) }
.omega-nth-table{  @include omega(4n, table); }
.omega-nth-default-left{  @include omega(4n, block, left); }
.omega-nth-table-left{  @include omega(4n, table, left); }

and I got the error:

Syntax error: Mixin omega takes 2 arguments but 3 were passed.

After I tested without 3 arguments:

//.omega-nth-default-left{  @include omega(4n, block, left); }
//.omega-nth-table-left{  @include omega(4n, table, left); }

and I got it:

passed, .omega-default has margin-right equal 0
passed, .omega-table has padding-right equal 0
passed, .omega-default-left has margin-left equal 0
passed, .omega-table-left has padding-left equal 0
passed, .omega-nth-default:nth-child(4n) has margin-right equal 0
expected `.omega-nth-table:nth-child(4n)` with `padding-right:0` but failed, got only `["margin-table: 0;"]`
expected `.omega-nth-default-left:nth-child(4n)` with `margin-left:0` but failed, got only `[]`
expected `.omega-nth-table-left:nth-child(4n)` with `padding-left:0` but failed, got only `[]`

There are problems with the nth-omega and 3 arguments

@kaishin
Admin

The error is coming from the two last. The right syntax to use is:

.omega-nth-default-left{  @include omega(4n block, left); }
.omega-nth-table-left{  @include omega(4n table, left); }

Once, we get @treppo's auto-table detection merged in, using table will become optional.

@kaishin kaishin closed this
@dukex dukex deleted the branch
@treppo

great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 11, 2012
  1. @dukex
  2. @dukex
Commits on Dec 16, 2012
  1. @dukex

    fixed typo

    dukex authored
This page is out of date. Refresh to see the latest.
Showing with 15 additions and 18 deletions.
  1. +15 −18 app/assets/stylesheets/grid/_grid.scss
View
33 app/assets/stylesheets/grid/_grid.scss
@@ -89,28 +89,25 @@ $fg-max-width: $max-width;
}
// Remove element gutter
-@mixin omega($display: block, $direction: right) {
- @if $display == table {
- padding-#{$direction}: 0;
- }
-
- @else {
- margin-#{$direction}: 0;
+@mixin omega($display_or_nth: block, $direction_or_display: right, $direction: right) {
@kaishin Admin
kaishin added a note

That's an interesting solution, however I'm not sure about using one argument for two different things. In cases such as this one, I tend to favor the flexibility of list arguments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ @if $display_or_nth == table {
+ padding-#{$direction_or_display}: 0;
+ } @else if $display_or_nth == block {
+ margin-#{$direction_or_display}: 0;
+ } @else {
+ &:nth-child(#{$display_or_nth}) {
+ @if $direction_or_display == table {
+ padding-#{$direction}: 0;
+ } @else {
+ margin-#{$direction}: 0;
+ }
+ }
}
}
@mixin nth-omega($nth, $display: block, $direction: right) {
- @if $display == table {
- &:nth-child(#{$nth}) {
- padding-#{$direction}: 0;
- }
- }
-
- @else {
- &:nth-child(#{$nth}) {
- margin-#{$direction}: 0;
- }
- }
+ @warn "The nth-omega() mixin was deprecated. Please use omega()";
+ @include omega($nth, $display, $direction);
}
// Fill 100% of parent
Something went wrong with that request. Please try again.