-
Notifications
You must be signed in to change notification settings - Fork 625
Remove CircleBadge
component
#6674
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 11caf8a 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! |
size-limit report 📦
|
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 the CircleBadge
component from the Primer React design system as part of a major version release. The removal is comprehensive, eliminating all component files, tests, stories, documentation, and related exports.
- Completely removes the
CircleBadge
component and its subcomponentCircleBadge.Icon
- Updates export files to remove
CircleBadge
references from both main and styled-react packages - Removes all associated test files, stories, and e2e test configurations
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
script/generate-e2e-tests.js |
Removes CircleBadge from e2e test configuration |
packages/styled-react/src/index.ts |
Removes CircleBadge export from styled-react package |
packages/styled-react/src/__tests__/__snapshots__/exports.test.ts.snap |
Updates export snapshot to reflect removal |
packages/react/src/index.ts |
Removes CircleBadge component and type exports |
packages/react/src/__tests__/__snapshots__/exports.test.ts.snap |
Updates export snapshot to reflect removal |
packages/react/src/CircleBadge/index.ts |
Deletes component index file |
packages/react/src/CircleBadge/__snapshots__/CircleBadge.test.tsx.snap |
Deletes test snapshots |
packages/react/src/CircleBadge/CircleBadge.types.test.tsx |
Deletes TypeScript type tests |
packages/react/src/CircleBadge/CircleBadge.tsx |
Deletes main component implementation |
packages/react/src/CircleBadge/CircleBadge.test.tsx |
Deletes unit tests |
packages/react/src/CircleBadge/CircleBadge.stories.tsx |
Deletes Storybook stories |
packages/react/src/CircleBadge/CircleBadge.docs.json |
Deletes component documentation metadata |
e2e/components/CircleBadge.test.ts |
Deletes e2e visual regression tests |
.changeset/pretty-flies-cheer.md |
Documents the breaking change for both packages |
'@primer/react': major | ||
--- | ||
|
||
CircleBadge: Remove component `CircleBadge` |
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 changelog entry has an inconsistent description. It should be 'Remove CircleBadge
component' to match standard changelog format and avoid redundancy.
CircleBadge: Remove component `CircleBadge` | |
Remove `CircleBadge` component |
Copilot uses AI. Check for mistakes.
Hi 👋 Should we move this to deprecated in case someone outside of github/github-ui is using it? |
@siddharthkp, I think we've just been removing them outright from PRC and skipping the deprecation step, at least for the smaller components with low usage. We could include a migration guide, which would advise consumers to utilize the styles we used for the component, but I'm not sure if that'd be helpful or not 🤔 Let me know what you think! |
(I feel 50% strongly about this, not 100%) Looking at primer-query, there are a few repos that use it and these are the kind of repos that get stuck on old primer versions because we don't help them upgrade 😅. The feedback we get from these teams is that primer is really hard to upgrade, so I have some guilt around it. I'd recommend moving them to deprecated for at least one major, that's what we usually promise (and hopefully it's not a huge time commitment). But I'd also not want to block your progress, so I'll let you choose either direction. 😇 |
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.
Left some direction-ey feedback, approving in advance :)
Definitely makes sense. In retrospect, I think we should've deprecated the components as soon as we decided to "bonfire" them, as it would've given more time for folks to move away from those components. I'm indifferent to the approach we take, but might lean more towards removing it, only because we've already removed some for v38 😅. I do think we should follow the path of deprecating first then waiting for the next major. I can follow up with either a migration guide, or a lint rule that could help with existing usage outside of Dotcom if we end up removing this one. cc: @joshblack, curious what your take would be on this! |
I think my ideal would be:
For this one, I'm down either way. I don't think there's any harm in keeping these components around really. Also wanted to share if it helps with decision making, I recently added support for querying by version in Primer Query. Here's a link to projects on the current major of Primer React that are using this component: https://primer-query.githubapp.com/?query=name%3Acircleocticon+-repo%3Agithub%2Fgithub-ui+-repo%3Agithub%2Fprimer-docs+package%3A%22%40primer%2Freact%22+version%3A%3E%3D37.x If we're thinking about this release specifically, if we can help teams migrate that'd be great. If we don't have a clear migration story though then it might be worth us revisiting this to see what they should use instead and reassessing if there is a good path moving forward (I'm not caught up on context so let me know if that's already the case 👀) |
Closes https://github.com/github/primer/issues/5535
Removes
CircleBadge
from PRC.https://github.com/github/github-ui/pull/1113 needs to be merged before this PR.
Changelog
Removed
CircleOcticon
componentRollout strategy
Testing & Reviewing
Merge checklist