Skip to content
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

Merged
merged 16 commits into from
Oct 15, 2021
Merged

Bump @primer/primitives to v5 #1514

merged 16 commits into from
Oct 15, 2021

Conversation

colebemis
Copy link
Contributor

@colebemis colebemis commented Oct 14, 2021

Bumps @primer/primitives to v5 and addresses breaking changes in that release.

@changeset-bot
Copy link

changeset-bot bot commented Oct 14, 2021

🦋 Changeset detected

Latest commit: a9245bb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/components Major

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

@github-actions
Copy link
Contributor

github-actions bot commented Oct 14, 2021

size-limit report 📦

Path Size
dist/browser.esm.js 53.88 KB (-14.5% 🔽)
dist/browser.umd.js 54.25 KB (-14.43% 🔽)

@@ -55,7 +55,8 @@
{
"allow": ["dark_dimmed"]
}
]
],
"primer-react/no-deprecated-colors": ["warn", {"checkAllStrings": true}]
Copy link
Collaborator

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.

package.json Outdated Show resolved Hide resolved
@@ -44,7 +44,7 @@
"license": "MIT",
"dependencies": {
"@primer/octicons-react": "^13.0.0",
"@primer/primitives": "4.8.1",
"@primer/primitives": "5.1.0",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wohoo!

Comment on lines +60 to +66
// 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')};
Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor Author

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think neutral.subtle is the right move. For the IssueLabelToken one it seems neutral.muted works better because it darkens what is light and it lightens what is dark:
image

Copy link
Contributor Author

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 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants