-
Notifications
You must be signed in to change notification settings - Fork 638
Update Token
close button sizing
#7017
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: d54010f The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
Uh oh! @langermank, at least one image you shared is missing helpful alt text. Check your pull request body to fix the following violations:
Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
|
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.
Pull Request Overview
This PR fixes the close button icon sizing in the Token component to address visual inconsistencies. The changes simplify the icon sizing logic and improve CSS consistency by using design tokens and removing redundant line-height declarations.
- Simplified icon sizing from a calculated approach to fixed sizes (12px for small/medium/large, 16px for xlarge)
- Standardized CSS by replacing hardcoded values with design tokens and consolidating line-height rules
- Updated color variables to use proper semantic tokens for interactive states
Reviewed Changes
Copilot reviewed 4 out of 86 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
packages/react/src/Token/_RemoveTokenButton.tsx | Simplified icon sizing logic and removed unused function |
packages/react/src/Token/_RemoveTokenButton.module.css | Updated to use design tokens and proper semantic color variables |
packages/react/src/Token/TokenBase.module.css | Consolidated line-height and replaced hardcoded values with design tokens |
.changeset/fresh-turtles-carry.md | Added changeset documentation for the patch |
}} | ||
> | ||
<XIcon size={getTokenButtonIconSize(size)} /> | ||
<XIcon size={size === 'small' || size === 'medium' || size === 'large' ? 12 : 16} /> |
Copilot
AI
Oct 16, 2025
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.
[nitpick] The repeated conditional logic for icon sizing could be extracted into a helper function or constant mapping for better maintainability and consistency.
Copilot uses AI. Check for mistakes.
type="button" | ||
> | ||
<XIcon size={getTokenButtonIconSize(size)} /> | ||
<XIcon size={size === 'small' || size === 'medium' || size === 'large' ? 12 : 16} /> |
Copilot
AI
Oct 16, 2025
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.
[nitpick] This duplicates the same conditional logic from line 40. Consider extracting this into a shared function or constant to avoid code duplication.
Copilot uses AI. Check for mistakes.
Closes https://github.com/github/primer/issues/5319
Fixes close button icon sizing