-
Notifications
You must be signed in to change notification settings - Fork 645
CounterLabel: Deprecated scheme prop in favor of variant prop #7185
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: 0517d2c 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 deprecates the scheme prop in favor of the variant prop for the CounterLabel component, aligning with the design system's naming conventions while maintaining backward compatibility.
- The
variantprop is introduced as the new API with the same values ('primary' | 'secondary') - The
schemeprop is marked as deprecated with JSDoc annotation - Fallback logic ensures
varianttakes precedence overscheme, with 'secondary' as the default
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| CounterLabel.tsx | Added variant prop, deprecated scheme prop, implemented fallback logic with precedence handling, updated data attribute from data-scheme to data-variant |
| CounterLabel.test.tsx | Added tests for the new variant prop (primary and secondary), updated test description to reflect both props, maintained backward compatibility tests for scheme |
| CounterLabel.stories.tsx | Updated Playground story to use variant prop instead of scheme in default args |
| CounterLabel.features.stories.tsx | Updated PrimaryTheme story to use variant prop, kept SecondaryTheme using scheme for backward compatibility demonstration |
| CounterLabel.module.css | Updated CSS selectors from data-scheme to data-variant for primary and secondary styles |
| .changeset/busy-masks-kiss.md | Added changeset documenting the deprecation as a minor version change |
Comments suppressed due to low confidence (3)
packages/react/src/CounterLabel/CounterLabel.test.tsx:48
- This test should verify that the default
data-variantattribute is set to 'secondary'. Add an assertionexpect(container.firstChild).toHaveAttribute('data-variant', 'secondary')to validate the default behavior.
it('renders with secondary variant when no "scheme" or "variant" prop is provided', () => {
const {container} = HTMLRender(<CounterLabel>1234</CounterLabel>)
expect(container.firstChild).toBeInTheDocument()
expect(container.firstChild).toHaveTextContent('1234')
})
packages/react/src/CounterLabel/CounterLabel.test.tsx:48
- Missing test case for precedence: when both
variantandschemeprops are provided,variantshould take precedence. Add a test likeit('gives precedence to variant over scheme when both are provided', () => { const {container} = HTMLRender(<CounterLabel variant=\"primary\" scheme=\"secondary\">1234</CounterLabel>); expect(container.firstChild).toHaveAttribute('data-variant', 'primary'); })to verify this critical behavior.
})
packages/react/src/CounterLabel/CounterLabel.stories.tsx:21
- The argTypes should include configuration for the new
variantprop in addition to the deprecatedschemeprop. This allows Storybook users to interact with the new API. Add avariantentry with the same control configuration to properly document both the new and deprecated props.
argTypes: {
scheme: {
control: 'select',
options: ['primary', 'secondary'],
},
},
siddharthkp
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.
Only minor feedback, approving in advance :)
| it('respects the secondary "variant" prop', () => { | ||
| const {container} = HTMLRender(<CounterLabel variant="secondary">1234</CounterLabel>) | ||
| expect(container.firstChild).toBeInTheDocument() | ||
| expect(container.firstChild).toHaveTextContent('1234') |
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.
Both of the added tests check for the same thing 🤔
What are we testing here?
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.
Fixed. I somehow just copied the old tests and didn't get that they don't quite make sense.
| const {container} = HTMLRender(<CounterLabel>1234</CounterLabel>) | ||
| expect(container.firstChild).toBeInTheDocument() | ||
| expect(container.firstChild).toHaveTextContent('1234') | ||
| }) |
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.
Maybe a good idea to add this test as well: "prefers variant prop over deprecated scheme prop"
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.
Added a test for that.
Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com>
Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com>
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/6782 |
|
🟢 ci completed with status |
siddharthkp
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, ship it
Closes https://github.com/github/primer/issues/5274
Changelog
CounterLabel: Deprecated scheme prop in favor of variant prop
New
Changed
Removed
Rollout strategy
Testing & Reviewing
Merge checklist