-
Notifications
You must be signed in to change notification settings - Fork 645
Token: Fix sizing, padding, and selected state styling #7165
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
Conversation
🦋 Changeset detectedLatest commit: bba3edb 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 |
|
👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
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.
Pull Request Overview
This PR addresses multiple styling issues in the Token component, focusing on size-specific adjustments for icon sizing, padding, font sizes, and visual treatment of selected tokens. The changes also prevent the leading visual from rendering when the token size is 'small'.
- Adjusted remove button icon size to use 16px for 'large' and 'xlarge' sizes (previously all three non-small sizes used 12px)
- Updated padding values for 'medium' and 'xlarge' token sizes
- Changed font size for 'large' tokens from small to medium
- Added border styling for selected tokens
- Disabled leading visual rendering for 'small' tokens
Reviewed Changes
Copilot reviewed 6 out of 150 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
packages/react/src/Token/_RemoveTokenButton.tsx |
Updated icon size logic to use 16px for 'large' and 'xlarge' tokens |
packages/react/src/Token/TokenBase.module.css |
Adjusted padding for 'medium' and 'xlarge' sizes, updated font size for 'large' |
packages/react/src/Token/Token.tsx |
Added comment and conditional logic to prevent leading visual on 'small' tokens |
packages/react/src/Token/Token.module.css |
Added border styling for selected tokens |
.playwright/snapshots/**/*.png |
Updated visual regression test snapshots |
.changeset/cuddly-worms-knock.md |
Added changeset for patch release |
|
Hey @liuliu-dev, sorry, we need a little change:
|
|
@liuliu-dev Nitpick, but could you please set the following storybook properties to
This will make it nicer for users:
Just to clarify, this is NOT a bug or something you introduced, but since we are touching the component we may use the opportunity to improve this. 🙏 |
|
👋 Hi, there are new commits since the last successful integration test. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/6646 |
|
🟢 ci completed with status |
|
Hey @lukasoppermann , I've updated the padding/margin, and the storybook. You can view it here: |
lukasoppermann
left a comment
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 great! Thank you. 🚀
TylerJDev
left a comment
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! Left feedback, but non-blocking!
| export interface TokenProps extends TokenBaseProps { | ||
| /** | ||
| * A component that renders before the token text | ||
| * disabled when size is 'small' |
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.
Does this mean that any tokens using leading visuals and have a size of "small" will no longer render the leading visual?
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 correct! as requested in the issue:
"small variant of the token should use not allow for leadingVisuals"
.changeset/cuddly-worms-knock.md
Outdated
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@primer/react': patch | |||
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.
Depending on my last comment, maybe this is worth a minor? But let me know what you think! 😁
|
👋 Hi, there are new commits since the last successful integration test. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |


Fixes multiple styling issues in the Token component to match Figma design.
Closes #5387
Changelog
Changed
--text-body-size-smallto--text-body-size-mediumleadingVisualsupport (now only renders when size is not 'small')border-style: solidandborder-color: var(--borderColor-emphasis))Rollout strategy
Testing & Reviewing
Merge checklist