-
Notifications
You must be signed in to change notification settings - Fork 355
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(generate tokens): prefix tokens with a t_ #11002
Conversation
Preview: https://patternfly-react-pr-11002.surge.sh A11y report: https://patternfly-react-pr-11002-a11y.surge.sh |
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! I did have to do a search and replace for @patternfly/react-tokens/dist/esm/global_
with @patternfly/react-tokens/dist/esm/t_global_
in react-core/src
and react-charts/src
to get the build to work but the t_*
tokens look to have all generated correctly 👍
@@ -10,7 +10,7 @@ const version = 'v6'; | |||
|
|||
// Helpers | |||
const formatCustomPropertyName = (key) => | |||
key.replace(`--pf-${version}-`, '').replace('--pf-t--', '').replace(/-+/g, '_'); | |||
key.replace(`--pf-${version}-`, '').replace('--pf-t--', 't_').replace(/-+/g, '_'); |
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 curious why the version has been removed?
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 built this branch locally and react-tokens still generates duplicates. The only difference now is one file is prefixed with "t_".
For example:
packages/react-tokens/dist/esm/chart_global_label_Margin.js
packages/react-tokens/dist/esm/t_chart_global_label_margin.js
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.
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.
Hey @dlabrecq. The original bug is because the PF CSS has variables named very similarly like these two - --pf-v6-chart-global--label--Margin
and --pf-t--chart--global--label--margin
. The react-tokens package parses the PF CSS, finds all of our variables, and creates a react-token for each one with a name based based off of what the CSS variable name is. Most of the PF CSS vars fit into 4 categories:
- Components -
--pf-v6-c-[component]
creates a react token likec_[component]
- Layouts -
--pf-v6-l-[layout]
, createsl_[layout]
- Charts -
--pf-v6-chart-[color/global/axis/bar/etc]
, createschart_[color/global/axis/bar/etc]
- Tokens, aka global vars -
--pf-t--[color/global/chart/etc]
, creates[color/global/chart/etc]_
So for the vars reference above, --pf-v6-chart-global--label--Margin
and --pf-t--chart--global--label--margin
, the resulting tokens are chart_global_label_Margin
and chart_global_label_margin
. There are a bunch of ways we could address that, but since the CSS token vars (--pf-t--[...]
) are always prefixed with -t--
, prefixing the react token filename with t_[...]
solves this problem and follows the format of other prefixed vars/tokens in PF (--pf-v6-c
and --pf-v6-l
).
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.
Thanks for the explanation. I didn't realize we wanted to expose both tokens and variables in this package
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 - thanks for the notes on testing and the additional context from @mcoker
@tlabaj Can you link the org PR and then we can merge this. |
c60444b
to
328d54f
Compare
328d54f
to
dcfcb7a
Compare
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #10985
A issue was noticed that react-token created chart token exports that differed by one uppercase character. example
export { chart_global_label_Margin } from './chart_global_label_Margin'; export { chart_global_label_margin } from './chart_global_label_margin';
To test pull down this branch, build and look at the
react-tokens
dist
directory.eg. of token before change
after change
@thatblindgeye could we write code mod for this?