-
Notifications
You must be signed in to change notification settings - Fork 29
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
Remove unused tokens #515
base: main
Are you sure you want to change the base?
Remove unused tokens #515
Conversation
🦋 Changeset detectedLatest commit: 0b91e58 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 design token changes found |
🟢 No visual differences foundOur visual comparison tests did not find any differences in the UI. |
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.
Looks good to me. However, I'm unsure if the --brand-animation-variant-scaleInTop-distance
was intentionally left unused or not. Either way, we can bring it back if needed later.
While these tokens aren't used internally, I'd say there's a high likelihood that they're referenced by the end-user somewhere. E.g. I know that we reference Removing these tokens technically constitutes a breaking change, so wondering: a) do we need to do this right now? Can it be deferred and batched with the other upcoming breaking changes? |
@rezrah what is the plan for upgrading packages that consume Primer Brand tokens when we complete the mass renaming? I think the last time we talked about it, you weren't concerned about custom CSS usage. Since I'm adding a stylelint config/plugin to handle find and replace for all the other tokens, I can do the same for these. But it would be helpful if I can see where they are used to best recommend a replacement token. |
For tokens, a combination of stylelint codemods and/or written migration guides in the changelog should be enough, I think 👍 Here's a very recent example of a functional token being used consumer-side.
🙏 That sounds great. If the configs aren't ready / available.. could also just expand the changelog entry to include a migration Leaving approval in meantime, as I'll be out until the new year. |
"secondary": { | ||
"value": "var(--base-color-scale-purple-5)", | ||
"dark": "var(--base-color-scale-purple-3)" |
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.
Looks like we may need additional accent color variables in future to support our brand pillars, so worth keeping this around?
Summary
Part of https://github.com/github/primer/issues/2097
I've got a lot of changes coming soon, so just removing anything that's not actually used to keep things cleaner. We have a better strategy in place for these kinds of generic tokens, but I haven't run into a use case to add them here.
List of notable changes:
Removed unused tokens:
Contributor checklist:
Reviewer checklist: