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

Padding argument on span-columns mixin #85

Merged
merged 3 commits into from
Aug 9, 2012
Merged

Conversation

tbredin
Copy link

@tbredin tbredin commented Aug 8, 2012

  • Accepts 1 or 2 values and applies inner padding on grids
    • If only one value is passed then padding will be identical on both sides
    • Padding is applied in from-to order, based on the $from argument
    • $from argument is now in the 4th position (whereas it used to be the 3rd). This may need to be changed

Added relative-width function for calculating percentage widths from any given value

  • Amended column, columns and gutter functions to use this new function

…padding on grids

 - Accepts 1 or 2 values and applies inner padding on grids
 - If only one value is passed then padding will be identical on both sides
 - Padding is applied in from-to order, based on the $from argument
 - $from argument is now in the 4th position (whereas it used to be the 3rd). *This may need to be changed*

Added relative-width function for calculating percentage widths from any given value
 - Amended column, columns and gutter functions to use this new function
$context : $total-columns,
$from : $from-direction
$context : $total-columns,
$padding : 0em,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This default needs to take its units from the settings, or we'll get an error when someone builds a non-em-based grid.

@mirisuzanne
Copy link
Member

Very nicely done! That's a first pull-request you should be proud of! I hope you contribute more!

I made a few notes above that I think would clean it up. It would also be great if you ran the tests and updated both tests and docs. There's no particular hurry on my end, so whenever you have time.

Thanks!

@tbredin
Copy link
Author

tbredin commented Aug 8, 2012

Is multiplying zero by a number of the correct units an accepted method for forcing units onto a unitless number?

Probably not that tidy... but I've struggled to find any other way to do it! Suggestions most welcome!

…dding test

- Altered the padding argument to accept values in the correct units (based on $gutter-width units). This is somewhat hacky at this stage.
$context : $total-columns,
$from : $from-direction
$context : $total-columns,
$padding : 0 * $gutter-width, // forces use of $gutter-width unit without producing a string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is a perfectly acceptable way to accomplish what I asked. That's how I would do it. But now that I see the test results, it seems to me the default should be false instead of 0, and we should only apply any padding values if they are passed. So you could still pass 0em in order to remove padding, but the default should not do anything to the padding.

…when it is specified

- Amended reference description on $padding in span-columns mixin to be more specific
mirisuzanne added a commit that referenced this pull request Aug 9, 2012
Padding argument on span-columns mixin
@mirisuzanne mirisuzanne merged commit 71c05e7 into oddbird:master Aug 9, 2012
@mirisuzanne
Copy link
Member

Thanks! This was a great help, I hope you stick around and continue to contribute.

@tbredin
Copy link
Author

tbredin commented Aug 9, 2012

No problem! Yeah well we look to be adopting susy for future projects in the company, so I may well be back with some more contributions :)

Just as a final note I just realised I forgot to update the comment above the mixin - have updated and will commit now

@mirisuzanne
Copy link
Member

May want to pull in the latest first. I made one change locally.

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

Successfully merging this pull request may close these issues.

2 participants