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

Already on GitHub? Sign in to your account

added more flexibility to pad mixin #23 #25

Merged
merged 12 commits into from Dec 28, 2012

Conversation

Projects
None yet
3 participants
Contributor

dukex commented Nov 3, 2012

Added more flexibility to pad mixin, there are back compatibility with the old pad mixin behavior

Example:

.normal{  @include pad; } 
.override-all-1-with-default{ @include pad(default); }
.override-all-2-with-default{ @include pad(30px default); }
.override-all-3-with-default{ @include pad(default 20px 10px); }
.override-all-4-with-default{ @include pad(30px 20px 10px default); }

Output:

.normal {
  padding: 2.35765%; }

.override-all-1-with-default {
  padding: 2.35765%; }

.override-all-2-with-default {
  padding: 30px 2.35765%; }

.override-all-3-with-default {
  padding: 2.35765% 20px 10px; }

.override-all-4-with-default {
  padding: 30px 20px 10px 2.35765%; }

@dukex dukex referenced this pull request Nov 3, 2012

Closed

A More Flexible Pad Mix-in #23

Contributor

dukex commented Nov 7, 2012

@kaishin What do you think about that pull request?

@kaishin kaishin and 1 other commented on an outdated diff Nov 7, 2012

app/assets/stylesheets/grid/_grid.scss
@@ -85,7 +85,12 @@ $fg-max-width: $max-width;
// Pad
@mixin pad($padding: flex-gutter()) {
- padding: $padding;
+ $_padding: null;
@kaishin

kaishin Nov 7, 2012

Can we rename this variable to $padding-list for clarity and consistency?

@dukex

dukex Nov 7, 2012

Contributor

Done!

@kaishin kaishin and 1 other commented on an outdated diff Nov 7, 2012

app/assets/stylesheets/grid/_grid.scss
@@ -85,7 +85,12 @@ $fg-max-width: $max-width;
// Pad
@mixin pad($padding: flex-gutter()) {
- padding: $padding;
+ $_padding: null;
+ @each $value in $padding {
+ $value: if(type_of($value) != 'number', flex-gutter(), $value);
@kaishin

kaishin Nov 7, 2012

Can we the type-of syntax here?

@dukex

dukex Nov 7, 2012

Contributor

I used type-of to get when value is default, do you think is better use $value == 'default' instead?

kaishin commented Nov 7, 2012

Other than the comments above, it's looking good!

dukex and others added some commits Nov 7, 2012

Contributor

dukex commented Nov 12, 2012

Removed type_of

kaishin commented Nov 12, 2012

Is there a reason why you made the last change (taking only default as value)?

Contributor

dukex commented Nov 12, 2012

Yes, there is. After the last change pad mixin got all params isn't a number and replace by default value, this can be good but don't make sense if the documentation say to use 'default' I will use 'default' and not 'chessebuger'. About performance, maybe a string match is faster than a type match in sass (I think)

Anyway, I think now the code is a little bit more legible.

kaishin commented Nov 12, 2012

Fair enough.

kaishin commented Dec 28, 2012

@dukex Please rename padding_list to padding-list and rebase against master so that I can merge.

Contributor

dukex commented Dec 28, 2012

Done!

kaishin commented Dec 28, 2012

Awesome, thanks!

@kaishin kaishin merged this pull request into thoughtbot:master Dec 28, 2012

@kaishin kaishin pushed a commit that referenced this pull request Dec 28, 2012

Reda Lemeden Add flexibility to pad() #25 23b51f2

kaishin commented Dec 28, 2012

@dukex FYI, after using the Merge button master ended up with lots of duplicate commits so I had to squash the commits in your branch and rebase against master to avoid this. I will be adding a contribution file with more details on this workflow.

Thanks again!

Contributor

dukex commented Dec 28, 2012

Nice @kaishin, about commits problems don't worry

@dukex dukex deleted the dukex:feature/pad-mode-flexible branch Dec 28, 2012

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