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

Make small changes and additions to variables #80

Merged
merged 3 commits into from
Jul 23, 2014
Merged

Conversation

kylefiedler
Copy link
Contributor

  • Rearrange size variables
  • Add z-index variables
  • Make header-line-height unitless

$unitless-header-line-height: $header-line-height / ($header-line-height * 0 + 1); // Strip units from line-height: https://developer.mozilla.org/en-US/docs/Web/CSS/line-height#Prefer_unitless_numbers_for_line-height_values

// Other Sizes
$base-border-radius: em(3);
Copy link

Choose a reason for hiding this comment

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

This is one of the few dimensions that I like a consistent value that's not relative to font-size. Maybe rem(3)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rem(3) would also force Bourbon 4.0 and Sass 3.3.

I get what you are saying though. If someone bumps up the font size you don't want the radius to change. 3rem or 3px?

Copy link

Choose a reason for hiding this comment

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

I'd prefer 3px to em(3). Like if code has a $small-font-size and a border with radius, it's no longer consistent with the rest of the app.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I much prefer border radius to be specified in pixels.

@creuter
Copy link

creuter commented Jun 17, 2014

👍 Wish 3.3 everywhere.

@@ -6,14 +6,14 @@ table {
}

th {
border-bottom: 1px solid darken($base-border-color, 15%);
border-bottom: 1px solid darken($base-border-color, 15);
Copy link

Choose a reason for hiding this comment

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

It's fine if deliberate, but these don't output the same values.

Copy link

Choose a reason for hiding this comment

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

^ Disregard, I'm dumb

@creuter
Copy link

creuter commented Jun 17, 2014

💸

// Line height
$base-line-height: $base-font-size * 1.5;
$unitless-line-height: $base-line-height / ($base-line-height * 0 + 1); // Strip units from line-height: https://developer.mozilla.org/en-US/docs/Web/CSS/line-height#Prefer_unitless_numbers_for_line-height_values
$header-line-height: $base-font-size * 1.25;
Copy link
Contributor

Choose a reason for hiding this comment

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

Where do see this being used now that there is $unitless-header-line-height? I feel as though the line-heights are getting overly complicated.

@tysongach
Copy link
Contributor

Merge, merge, merge 😃

@kylefiedler kylefiedler merged commit 991d5bb into master Jul 23, 2014
@kylefiedler kylefiedler deleted the kf-variables branch July 23, 2014 19:15
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.

5 participants