-
Notifications
You must be signed in to change notification settings - Fork 138
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
docs(tokens) Updates migration instructions. #4239
Conversation
@srambach RE #4218, were you thinking of a different direction than this pr goes? Much of the info from the linked doc in the issue was already on the site, but do you think it would make more sense in a different page or with more detail? Added some more details and specifics, though! I saw the codemods notes in the original doc, but they seemed like observations more than things we want to communicate on org |
I think this came out of a recent conversation specifically about React tokens (which are the React version of our CSS variables). This includes more than just token variables. This is great info, and I like the improvements you've made to the "develop with tokens" page. But I think this was intended to add some information to the upgrade guide about finding and fixing React tokens specifically. @nicolethoen @wise-king-sullyman maybe you two have more specific information? |
Following on what Sarah has written about React tokens - we just discussed that on a working session, and it would be great to add some notes like this:
|
@adamviktora Is this a blocker or could it be done in a follow up? |
@kmcfaul It can be a follow up for sure |
@kmcfaul I think @adamviktora 's suggestions are the core of this issue so I think it's worth doing before merging this rather than making another issue. |
@adamviktora ty for this context! If we do want to go further in these docs, or if I'm misunderstanding any of these details, it would probably be good to arrange time for a call to help me understand this more deeply (also cc @nicolethoen) ! |
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 really like the updates, I think it explains React tokens pretty well. Two comments bellow:
packages/documentation-site/patternfly-docs/content/get-started/upgrade.md
Outdated
Show resolved
Hide resolved
@@ -61,7 +61,7 @@ PatternFly 6 supports our new design token system, which changes variable names | |||
|
|||
Wherever you have any custom CSS overrides that reference PatternFly class names or CSS variables, you should carefully review them and make updates to ensure that they align with our token variables, which are outlined in our [tokens documentation](/tokens/all-patternfly-tokens). As much as possible, we recommend removing your CSS overrides so that your product upgrade experience will be smoother for future releases. | |||
|
|||
If your product uses a custom solution to replicate PatternFly styling (without using PatternFly components), then it will need to be reskinned. We recognize that this may be a large undertaking, so we encourage you to reach out to the PatternFly team so that we support this work. | |||
If your product uses a custom solution to replicate PatternFly styling (without using PatternFly components), then it will need to be re-skinned. We recognize that this may be a large undertaking, so we encourage you to reach out to the PatternFly team so that we can help support this work. | |||
|
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'd mention also (probably in this block) that we have a codemod for updating global CSS variables, which are part of their React code (it will not fix .css
files, but only javascript or typescript ones).
This codemod updates global color variables to a temporary hot pink color: --pf-t--temp--dev--tbd
(or t_temp_dev_tbd
when using React tokens), so consumers can build their code and visually see on the website, at which places they will have to manually replace this token with a new one.
For other global variables (spacers, font size, box shadow), it will try to provide an autofix to match the same value (or the closest one).
This codemod works both for CSS variables and React tokens.
e.g. a CSS variable: --pf-v5-global--FontSize--lg
becomes --pf-t--global--font--size--lg
and a react token: global_FontSize_lg
becomes t_global_font_size_lg
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 all of this info!! Is that this codemod? https://github.com/patternfly/pf-codemods?tab=readme-ov-file#tokens-update. I linked to that one in my last commit, but can adjust if needed. Is https://github.com/patternfly/pf-codemods/tree/main/packages/class-name-updater limited to css?
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.
Yes, that's the codemod. The class-name-updater is not limited to just css. Thanks for doing all the updates!
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 🎉
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.
I think this looks good, thank you! 🩷
Closes #4218, adds to existing tokens docs