-
Notifications
You must be signed in to change notification settings - Fork 535
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
Bump @primer/primitives to v5 #1514
Conversation
🦋 Changeset detectedLatest commit: a9245bb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
size-limit report 📦
|
@@ -55,7 +55,8 @@ | |||
{ | |||
"allow": ["dark_dimmed"] | |||
} | |||
] | |||
], | |||
"primer-react/no-deprecated-colors": ["warn", {"checkAllStrings": true}] |
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 see! it was only on mdx files previously.
@@ -44,7 +44,7 @@ | |||
"license": "MIT", | |||
"dependencies": { | |||
"@primer/octicons-react": "^13.0.0", | |||
"@primer/primitives": "4.8.1", | |||
"@primer/primitives": "5.1.0", |
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.
wohoo!
// TODO: choose a better functional color variable for this | ||
background-color: ${get('colors.neutral.muted')}; | ||
} | ||
|
||
&:active { | ||
background-color: ${get('colors.fade.fg15')}; | ||
// TODO: choose a better functional color variable for this | ||
background-color: ${get('colors.neutral.subtle')}; |
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.
@mperrotti Since fade.*
variables were removed in Primitives v5, I had to make a best guess at alternative functional variables. Can you check out the preview storybook and see if you're okay with these alternatives?
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.
These work well for the regular tokens, but they're not ideal for IssueLabelTokens that have dark background colors.
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 comfortable going forward with this as-is and improving in a follow-up
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.
Agreed. @auareyou do you have any ideas about how we can replace fade variables 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.
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.
Sweet. Let's move forward with this as-is for now 👍
Bumps @primer/primitives to v5 and addresses breaking changes in that release.