-
Notifications
You must be signed in to change notification settings - Fork 266
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(react-native): border widths in the theme don't throw runtime errors #4227
Conversation
🦋 Changeset detectedLatest commit: 82e9251 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
if (path[0] === 'borderWidths' || path[0] === 'space') { | ||
return parseFloat(value); | ||
} |
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.
To make the logic in setupToken
a bit easier to reason about in the future, what do you think abt combining the conditional block here the block immediately below and the block on line 43 and abstracting a util for checking whether the value should be passed to parseFloat
? e.g.
// contrived name
const shouldParseFloatValue = (pathKey: string) => pathKey === 'space' || pathKey === 'borderWidths' || pathKey === 'opacities';
const setupToken = (
// ...setupToken params
) {
// ...other setupToken logic
if (shouldParseFloat(path[0])) {
if (value.includes('rem')) {
return Math.floor(parseFloat(value) * 16 * spaceModifier);
}
return parseFloat(value);
}
}
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.
love it
borderWidths: { | ||
small: '4', | ||
medium: '1rem', | ||
large: 6, |
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.
Is this something specific in RN where this works without a unit? Or do we just not want it to break completely if people try to do it
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.
Specific to RN because when we use the theme we use it in a RN StyleSheet which is typed rather than outputting to a CSS string which is untyped. Most numeric values (space, borderWidth, fontSize, etc.) are expected to be a number rather than a string. In CSS its all just strings and numbers can be cast to strings but not the other way around.
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.
LGTM
Description of changes
Issue #, if available
Description of how you validated changes
Checklist
yarn test
passes and tests are updated/addedsideEffects
field updatedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.