fix(button): set default button heights #1719
Conversation
|
Deploy preview for the-carbon-components ready! Built with commit 78d8dbf https://deploy-preview-1719--the-carbon-components.netlify.com |
src/components/button/_mixins.scss
Outdated
| @@ -81,7 +81,7 @@ | |||
| flex-shrink: 0; | |||
| font-size: $button-font-size; | |||
| font-weight: $button-font-weight; | |||
| height: auto; | |||
| height: rem(48px); | |||
There was a problem hiding this comment.
IMO this should be rem($button-height) -- see: https://github.com/IBM/carbon-components/blob/e4da4a57542a42f7c6e0a90e91aac9b7e0fcaf02/src/globals/scss/_theme-tokens.scss#L140
| @@ -305,7 +305,7 @@ | |||
|
|
|||
| .#{$prefix}--btn--sm { | |||
| @include letter-spacing; | |||
| height: auto; | |||
| height: rem(32px); | |||
There was a problem hiding this comment.
Just pointing out that this is a bit different than how height is applied to small buttons in the non-experimental theme:
https://github.com/IBM/carbon-components/blob/98bae993a38f43dc45e6a23b5302f882926a0ab1/src/components/button/_button.scss#L163
There was a problem hiding this comment.
the output is identical (2rem) as our base font size is 16px, unless I am mistaken. Please correct me if that's not the case
There was a problem hiding this comment.
Yup, it's the same. Just pointing it out for consistency 👍
There was a problem hiding this comment.
yeah I figured I would use the rem conversion function to be consistent with the rest of the experimental styles
|
you're right @asudoh I overlooked that var and hardcoded its value instead. Should be fixed now |
|
🎉 This PR is included in version 9.70.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #1691
This PR sets default button heights to avoid collapsing when button content is not rendered