-
Notifications
You must be signed in to change notification settings - Fork 33
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
Upgrade to v9 primitives #717
Conversation
🦋 Changeset detectedLatest commit: 0fc2075 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
🟢 No visual differences foundOur visual comparison tests did not find any differences in the UI. |
🔍 Design token changes foundView CSS variable changes+ --base-size-6: 0.375rem; + --base-size-2: 0.125rem; - --brand-breakpoint-xxlarge: 90rem;
+ --brand-breakpoint-xxlarge: 87.5rem; - @custom-media --brand-viewportRange-wide-viewport (min-width: 90rem);
+ @custom-media --brand-viewportRange-wide-viewport (min-width: 87.5rem); |
* transform: scss variable names | ||
* example: `$namespace-item-variant-property-modifier` | ||
*/ | ||
StyleDictionary.registerTransform({ |
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.
You should be able to use this: https://github.com/primer/primitives/blob/main/src/transformers/namePathToKebabCase.ts
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.
🙌 nice..
added as a follow up PR item
* transform: css variable names | ||
* example: `--namespace-item-variant-property-modifier` | ||
*/ | ||
StyleDictionary.registerTransform({ |
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.
This can be removed, both scss and css are pathToKebabCase
, the prefix is added later.
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, added as a follow up PR item
* example: `namespace.item.variant.property.modifier` | ||
*/ | ||
StyleDictionary.registerTransform({ | ||
name: 'name/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.
You should be able to use this: https://github.com/primer/primitives/blob/main/src/transformers/namePathToDotNotation.ts
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, added as a follow up PR item. Will need the JS equivalent of that function to ship first.
* example: `NamespaceItemVariantPropertyModifier` | ||
*/ | ||
StyleDictionary.registerTransform({ | ||
name: 'name/js/es6', |
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.
We can upstream this if you want.
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 please 🙏
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.
Here we go: primer/primitives#1036
|
||
// transform: px to rem | ||
StyleDictionary.registerTransform({ | ||
name: 'pxToRem', |
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.
If you have token: "dimension"
on all your size tokens you could use: https://github.com/primer/primitives/blob/main/src/transformers/dimensionToRem.ts
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.
Needs a bit more investigation...
...added as a follow up PR item.
|
||
// ts output | ||
StyleDictionary.registerTransform({ | ||
name: 'attribute/typescript', |
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.
How does this work? Interesting. Can you explain 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.
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'll get back to you on that... 😅
...added as a follow up PR item.
|
||
// css output | ||
StyleDictionary.registerTransform({ | ||
name: 'attribute/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.
Why do you need this?
|
||
// transform: composite typography to shorthands | ||
StyleDictionary.registerTransform({ | ||
name: 'typography/shorthand', |
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.
We should be able to use this: https://github.com/primer/primitives/blob/main/src/transformers/typographyToCss.ts
If not, this should be our goal to make it use this upstream transformer.
// REGISTER THE CUSTOM TRANFORM GROUPS | ||
|
||
StyleDictionary.registerTransformGroup({ | ||
name: '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.
Just FYI, you don't need to use TransformGroups, you can just add the transforms to the platform directly.
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 meant to add this to scss, Although since scss and css are the same transforms, you could just use css
for all.
}) | ||
|
||
StyleDictionary.registerFormat({ | ||
name: 'custom/format/custom-media', |
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 you use this: https://github.com/primer/primitives/blob/main/src/formats/cssCustomMedia.ts ?
}, | ||
scss: { | ||
buildPath: `${outputPath}/scss/`, | ||
transformGroup: 'scss', |
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.
transformGroup: 'scss', | |
transformGroup: 'css', |
They are the same.
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.
We don't have CI diffing against the scss folder, so can't verify any regressions easily. Is this something you're super confident about @lukasoppermann, if so I can do it now. Alternatively, can do this one in a follow up PR.
pathToKebabCase, | ||
pathToDotNotation, | ||
capitalize, | ||
pathToPascalCase, |
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.
Do you need to export them?
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.
Probably not, let me remove these now 👍 thanks
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.
Comments can be addressed in a separate PR.
@@ -0,0 +1,495 @@ | |||
/** | |||
* Ported from primer/primitives, as it was removed in v8 release and we still need 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.
For posterity, this PR isn't where we'll refactor this file. That will happen in subsequent PRs. Tasklist added here.
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.
Looking great! This is a really exciting update! 🚀
Summary
Part of https://github.com/github/primer/issues/3176
Completes the upgrade to v9 primitives, keeping all previous build configuration in-tact.
Diff of changes can be seen in the bot comment here 👀
Note
All changes in this PR are directly related to the upgrade only and focussed around end-user output.
Refactoring of implementation details is out-of-scope, and will be reviewed at a later date to keep this PR manageable.
List of notable changes:
eslint-plugin-primer-react
dependency and subsequently impacted filesWhat should reviewers focus on?
Contributor checklist:
Reviewer checklist: