-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
chore(types): improve Link
types
#8081
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
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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 improves the type definitions for the Link
component by creating a union type that supports both localized links and standard anchor elements, allowing for additional properties like locale
. The changes simplify the Link component implementation while maintaining backward compatibility.
Key changes:
- Refactored
Link
component to use a union type (LinkProps
) that combinesComponentProps<typeof LocalizedLink>
andAnchorHTMLAttributes<HTMLAnchorElement>
- Simplified component implementations that use the Link component by removing redundant type definitions
- Removed unnecessary
kind="neutral"
props from Link usage in MinorReleasesTable
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
apps/site/components/Link.tsx |
Refactored to use union type for props and simplified component logic |
apps/site/components/LinkWithArrow.tsx |
Updated to use the new LinkProps type and added type assertion for Slot props |
apps/site/components/Common/Button.tsx |
Simplified type definition by removing redundant prop omissions |
apps/site/components/Releases/MinorReleasesTable/index.tsx |
Removed kind="neutral" props from Link components |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
LGTM, I will do the change in my PR once this got merged
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.
LGMT !
@avivkeller Could you help to merge this one ? And I thought I can do the change immediately |
@nodejs/nodejs-website requesting fast track |
Lighthouse Results
|
This PR expands the typings of
#site/components/Link
to allow for additional properties, likelocale
.