-
Notifications
You must be signed in to change notification settings - Fork 638
Replace theme=useTheme()
instances with default theme
#6897
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: b81af99 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! |
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 removes useTheme
hook usage and replaces it with direct theme imports as part of cleanup efforts. The primary purpose is to use default theme values instead of context-derived themes for specific tokens.
Key changes:
- Replaced
useTheme
hook with direct theme imports or CSS custom properties - Updated 5 specific tokens to use default theme values instead of custom theme context
- This is a breaking change that may affect external users with custom themes
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
packages/react/src/stories/useFocusZone.stories.tsx |
Replaced theme context usage with CSS custom property for accent color |
packages/react/src/UnderlineNav/styles.ts |
Converted function-based divider styles to static object with CSS custom properties |
packages/react/src/UnderlineNav/UnderlineNavContext.tsx |
Removed theme from context interface |
packages/react/src/UnderlineNav/UnderlineNav.tsx |
Removed theme context usage and updated divider style reference |
packages/react/src/SegmentedControl/SegmentedControl.tsx |
Replaced theme context with CSS custom property for border color |
packages/react/src/Overlay/Overlay.tsx |
Replaced theme context with direct theme import for space and animation values |
packages/react/src/LabelGroup/LabelGroup.tsx |
Replaced theme context with direct theme import for space value |
packages/react/src/ConfirmationDialog/ConfirmationDialog.features.stories.tsx |
Replaced theme context with CSS custom property for success color |
.changeset/replace-usetheme-with-theme.md |
Added changeset documenting the breaking change |
const {theme} = useTheme() | ||
const slideAnimationDistance = parseInt(get('space.2')(theme).replace('px', '')) | ||
const slideAnimationEasing = get('animation.easeOutCubic')(theme) | ||
const slideAnimationDistance = parseInt(theme.space[2], 10) |
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.
The parseInt
function should specify a radix (base 10) to avoid potential parsing issues. While you've added the radix parameter, the original code was parsing a string with 'px' suffix, but theme.space[2]
may not have the 'px' suffix that the original code was handling with .replace('px', '')
.
const slideAnimationDistance = parseInt(theme.space[2], 10) | |
const slideAnimationDistance = parseInt(String(theme.space[2]).replace('px', ''), 10) |
Copilot uses AI. Check for mistakes.
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.
This is fine because theme.space[2]
is hardcoded to '8px'
parseInt('8px', 10) = 8
theme=useTheme()
instances with default theme
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/3859 |
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.
✨
🟢 ci completed with status |
theme = useTheme()
usage with defaulttheme
.These are the tokens that are swapped from custom theme to default theme:
This means that if a custom
theme
is passed toThemeProvider
, which modifies any of these tokens, they will not be used. Instead the default primer theme will be used. This should mean no changes for github/github-ui, but might surprise external users that have changed any of the tokens above in their custom themes.Rollout strategy
Testing & Reviewing
Merge checklist