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

[K7] Font sizing, weights, families and mixins #12879

Merged
merged 4 commits into from
Jul 14, 2017

Conversation

snide
Copy link
Contributor

@snide snide commented Jul 14, 2017

Replaces #12876

Scaffolding and mixins for dealing with font sizing and weights. Dupes rems in so we can scale.

@snide snide requested a review from cjcenizal July 14, 2017 18:33
Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Just had some questions that didn't occur to me the first time around!

## Dealing with extends

You should try never to use the extends functionality in sass. The one exception is when you are extending placeholders to help control maintainabilty. General rule: if it creates an additive selector, it's usually a bad usage of extends.
Copy link
Contributor

Choose a reason for hiding this comment

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

Dumb question: what's an additive selector?


@mixin fontSize($size: $kuiFontSize) {
font-size: $size;
font-size: convert-to-rem($size);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a comment explaining why we're using both px and rem?

Copy link
Contributor

Choose a reason for hiding this comment

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

And why are we calling convert-to-rem when the name of the function is convertToRem?

$kuiFontSizeM: $kuiFontSize;
$kuiFontSizeL: 24px;
$kuiFontSizeXL: 32px;
$kuiFontSizeXXL: 48px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these other sizes be multiples of $kuiFontSize? So if $kuiSize changes, then everything changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, is there a reason not to convert these variables to rem here in their definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to still try and account for older versions of IE since we target enterprise. the fontSize mixin basically spits out both, but I need a base pixel value to output.

}

%kuiFontSizeXXL {
font-size: rem($kuiFontSizeXXL);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the rem function? Where does this come from and what's the difference with convertToRem?

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

😎 Cool!

@snide snide merged commit ab131c0 into elastic:k7-ui-framework Jul 14, 2017
@snide snide deleted the k7/fontsizing branch July 14, 2017 19:34
@snide snide added the Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. label Jul 17, 2017
cjcenizal pushed a commit to cjcenizal/kibana that referenced this pull request Jul 19, 2017
Sets up mixins and placeholders for typography.
cjcenizal pushed a commit to cjcenizal/kibana that referenced this pull request Aug 16, 2017
Sets up mixins and placeholders for typography.
cjcenizal pushed a commit to cjcenizal/kibana that referenced this pull request Aug 26, 2017
Sets up mixins and placeholders for typography.
cjcenizal pushed a commit that referenced this pull request Sep 19, 2017
Sets up mixins and placeholders for typography.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants