Skip to content

Conversation

siddharthkp
Copy link
Member

@siddharthkp siddharthkp commented Sep 9, 2025

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

@siddharthkp siddharthkp self-assigned this Sep 9, 2025
Copy link

changeset-bot bot commented Sep 9, 2025

⚠️ No Changeset found

Latest commit: fd422a6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added staff Author is a staff member integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Sep 9, 2025
Copy link
Contributor

github-actions bot commented Sep 9, 2025

👋 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!

Copy link
Contributor

github-actions bot commented Sep 9, 2025

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 89.47 KB (-0.17% 🔽)
packages/react/dist/browser.umd.js 89.57 KB (+0.01% 🔺)

@github-actions github-actions bot requested a deployment to storybook-preview-6817 September 9, 2025 20:14 Abandoned
@github-actions github-actions bot requested a deployment to storybook-preview-6817 September 9, 2025 20:21 Abandoned
@siddharthkp siddharthkp added the skip changeset This change does not need a changelog label Sep 9, 2025
@github-actions github-actions bot requested a deployment to storybook-preview-6817 September 9, 2025 20:30 Abandoned
@github-actions github-actions bot temporarily deployed to storybook-preview-6817 September 9, 2025 21:17 Inactive
@siddharthkp siddharthkp added integration-tests: skipped manually Changes in this PR do not require an integration test and removed integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Sep 10, 2025
@siddharthkp
Copy link
Member Author

integration-tests: skipped manually because I'm only adding a couple exports for hooks in primer/react, there are no modifications

@siddharthkp siddharthkp marked this pull request as ready for review September 10, 2025 11:11
@Copilot Copilot AI review requested due to automatic review settings September 10, 2025 11:11
@siddharthkp siddharthkp requested a review from a team as a code owner September 10, 2025 11:11
Copy link
Contributor

@Copilot Copilot AI left a 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 moves the ThemeProvider implementation from @primer/react to @primer/styled-react as part of a migration strategy. The main purpose is to create a standalone ThemeProvider implementation in the styled-react package that supports styled-components, while maintaining the API and functionality.

Key changes:

  • Added complete ThemeProvider implementation with theme utilities and color scheme support
  • Exported new hooks (useId, useSyncedState) from @primer/react to support the ThemeProvider
  • Updated test configuration to support both node and browser testing environments

Reviewed Changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
vitest.config.mts Updated test project paths to include both .ts and .mts browser config files
packages/styled-react/vitest.config.ts Added test name configuration for node environment
packages/styled-react/vitest.config.browser.ts Added test name configuration for browser environment
packages/styled-react/tsconfig.json Added browser vitest config to TypeScript includes
packages/styled-react/src/theming/utils/types/KeyPaths.ts Added utility type for generating dot-delimited key paths
packages/styled-react/src/theming/utils/theme.ts Added theme utility functions for font stacks, color/shadow partitioning
packages/styled-react/src/theming/themeGet.ts Added theme getter utility using styled-system
packages/styled-react/src/theming/theme.ts Added complete theme configuration with color schemes and type definitions
packages/styled-react/src/theming/ThemeProvider.tsx Added full ThemeProvider implementation with color mode switching
packages/styled-react/src/theming/README.md Added documentation explaining the theming folder purpose
packages/styled-react/src/index.tsx Updated exports to include theming utilities from local implementation
packages/styled-react/src/__tests__/ThemeProvider.browser.test.tsx Added comprehensive test suite for ThemeProvider functionality
packages/react/src/index.ts Exported useId and useSyncedState hooks for ThemeProvider usage
.prettierignore Added styled-react color schemes file to prettier ignore list

Comment on lines +1 to +4
// Temporarily disabling since this is originally a JavaScript that needed to be
// migrated to TypeScript for exports to work as correctly in Vite.
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-nocheck
Copy link
Preview

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

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

The @ts-nocheck directive disables all TypeScript checking for this file, which defeats the purpose of migrating to TypeScript. Consider removing this directive and properly typing the functions instead of bypassing type safety entirely.

Copilot uses AI. Check for mistakes.

const defaultNightScheme = 'dark'

// eslint-disable-next-line @typescript-eslint/no-explicit-any
export type Theme = {[key: string]: any}
Copy link
Preview

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

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

Using any type defeats TypeScript's type safety. Consider defining a more specific interface for the Theme type or using unknown if the structure is truly dynamic.

Suggested change
export type Theme = {[key: string]: any}
export type Theme = {[key: string]: unknown}

Copilot uses AI. Check for mistakes.

@siddharthkp
Copy link
Member Author

siddharthkp commented Sep 17, 2025

Realising this is maybe not the right step to start with 🤔 Going to go on a different path in another PR: #6867

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration-tests: skipped manually Changes in this PR do not require an integration test skip changeset This change does not need a changelog staff Author is a staff member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants