-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix(components): update type styles #1965
fix(components): update type styles #1965
Conversation
Deploy preview for the-carbon-components ready! Built with commit 3b09e3c https://deploy-preview-1965--the-carbon-components.netlify.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I just have to one question where theres a size and a type token
height: rem(40px); | ||
} | ||
|
||
.#{$prefix}--search--xl .#{$prefix}--search-input { | ||
@include typescale('epsilon'); | ||
@include type-style('body-short-02'); | ||
font-size: carbon--type-scale(4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is font-size: carbon--type-scale(4);
for and how is it different than the type style?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also unsure what this is used for
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @aagonzales @tw15egan good question - There seems no migration path for typescale('epsilon')
, and there seems no type-style()
corresponding to the font size for large search. That said, font-size: carbon--type-scale(4)
is for ensuring the font size is the one of large search. Does it make sense to you...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes makes sense but why is @include type-style('body-short-02');
still included then. Are you just overriding the size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you just overriding the size?
Yes, bigger font size, everything else is same as type-style('body-short-02')
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few small changes, otherwise good to go
@@ -266,8 +265,7 @@ | |||
position: absolute; | |||
margin-left: $spacing-lg; | |||
margin-top: rem(28px); | |||
@include font-size('12'); | |||
color: $text-02; | |||
@include type-style('label-01') color: $text-02; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be broken into two lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch @tw15egan thanks! Fixed.
height: rem(40px); | ||
} | ||
|
||
.#{$prefix}--search--xl .#{$prefix}--search-input { | ||
@include typescale('epsilon'); | ||
@include type-style('body-short-02'); | ||
font-size: carbon--type-scale(4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also unsure what this is used for
🎉 This PR is included in version 9.82.14 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Refs #1945.
Changelog
Changed
Testing / Reviewing
@IBM/carbon-designers: It'd be great if you can review focusing (only) on type tokens of the following components:
Thanks!
BTW did not touch the following components: