-
Notifications
You must be signed in to change notification settings - Fork 376
Add global field to react tokens #3832
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
|
PF3 preview: https://patternfly-react-pr-3832-pf3.surge.sh |
Codecov Report
@@ Coverage Diff @@
## master #3832 +/- ##
==========================================
+ Coverage 71% 71.01% +0.01%
==========================================
Files 785 785
Lines 10638 10642 +4
Branches 2314 2314
==========================================
+ Hits 7553 7557 +4
Misses 2655 2655
Partials 430 430
Continue to review full report at Codecov.
|
redallen
left a comment
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.
Step 1 is close! I've written something to help you get started on step 2 when you're ready:
const fs = require('fs');
const sassString = fs.readFileSync(require.resolve('@patternfly/patternfly/_variables.scss'), 'utf8');| const chartNum = decl.property.startsWith('--pf-chart-') && !isNaN(populatedValue); | ||
| const calculatedValue = chartNum ? Number(populatedValue).valueOf() : populatedValue; | ||
| tokens[key] = { | ||
| ...tokens[key], |
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 we assigning to tokens[key] more than once for each key? That isn't good.
| return computedValue ? computedValue.value : `var(${match})`; | ||
| }); | ||
| // Avoid stringifying numeric chart values | ||
| const chartNum = decl.property.startsWith('--pf-chart-') && !isNaN(populatedValue); |
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.
Can't we *just numerify all numbers that pass isNaN and remove the .startsWith('--pf-chart-') logic?
| }); | ||
| // Avoid stringifying numeric chart values | ||
| const chartNum = decl.property.startsWith('--pf-chart-') && !isNaN(populatedValue); | ||
| const calculatedValue = chartNum ? Number(populatedValue).valueOf() : populatedValue; |
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.
Can we move this logic to be on line 36?
| value: calculatedValue, | ||
| var: `var(${property})` | ||
| }; | ||
| const isGlobal = populatedValue.indexOf('--pf-global') > -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.
Nit: You can use .startsWith() and spread into tokens[key] = to reduce these 4 lines down to 1. Like
tokens[key ] = {
...(populatedValue.startsWith('--pf-global') && { global: populatedValue })
}|
CLosed in favor of #3896 |
This adds the global var that is referenced to get the final value.
Example:
What: Towards #3521
Additional issues: