-
Notifications
You must be signed in to change notification settings - Fork 65
Fix height prop to work with rem #347
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
Conversation
src/js/utils.js
Outdated
@@ -57,7 +57,9 @@ export function iconPad(props) { | |||
|
|||
let style = ''; | |||
if (height && theme?.text?.[height]?.height) { | |||
const lineHeight = parseMetricToNum(theme.text[height].height); | |||
const unit = theme.text[height].height.match(/[a-z]+$/)?.[0] || 'px'; |
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.
Why do we need both the regex and the ||
for px? I would think the regex would match both rem
and px
-- also I would think this would match any a-z chars such as em
,ft
, even zebra
.
Should we make the regex a bit more targeted to the specific units which are valid/we support?
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.
Made adjustments in latest commit.
src/js/utils.js
Outdated
const lineHeight = parseMetricToNum(theme.text[height].height); | ||
const unit = theme.text[height].height.match(/[a-z]+$/)?.[0] || 'px'; | ||
const lineHeight = | ||
parseMetricToNum(theme.text[height].height) * (unit === 'rem' ? 16 : 1); |
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.
Where does the 16 come from? Assuming this assumes and 16px root? Can we document this with a comment and/or a static variable (e.g. const ROOT_UNIT = 16
or ROOT_ELEMENT
)?
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.
Made adjustments in latest commit. It is worth noting, this PR does enhance height
to work with rem
but additional enhancements would be required to make it work with em
because we'd need to pass in the parent element node to get its font-size. I think we should leave that for separate work.
Co-authored-by: Matthew Glissmann <[email protected]>
Closes #346. If unit is
rem
, when we parseMetricToNum we need to multiply by 16 to appropriately calculate padding.Tested locally in storybook and added unit test.