-
Notifications
You must be signed in to change notification settings - Fork 646
BranchName: Update storybook #7182
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
|
|
👋 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 introduces stricter TypeScript typing for the BranchName component to ensure type safety around the href attribute. The component now enforces that href is required when rendering as an anchor tag (either explicitly with as="a" or by default), and disallows href when rendering as other elements.
Key changes:
- Added conditional type constraint requiring
href: stringwhenAs extends 'a'andhref?: neverotherwise - Updated default generic parameter to
'a'for better type inference - Added test coverage for the new type constraints using
@ts-expect-errorannotations
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/react/src/BranchName/BranchName.tsx | Added conditional type to enforce href requirement based on as prop value and set default generic parameter to 'a' |
| packages/react/src/BranchName/tests/BranchName.test.tsx | Updated existing tests to comply with new type constraints and added three new tests to verify type enforcement |
| .changeset/plenty-cloths-dance.md | Added changeset documenting the breaking type change as a patch release |
Co-authored-by: Copilot <[email protected]>
.changeset/plenty-cloths-dance.md
Outdated
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.
Don't need a changset if you are only updating stories. Just add skip changeset label to your PR :)
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 one change needed
Closes https://github.com/github/primer/issues/5270
Changelog
Updated storybook story to make sure we can test all instances of
BranchNameNew
Changed
Removed
Rollout strategy
Testing & Reviewing
Merge checklist