-
Notifications
You must be signed in to change notification settings - Fork 538
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
Migrate Header component to TSX #1020
Conversation
🦋 Changeset detectedLatest commit: 3524641 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 |
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/primer/primer-components/k5ehb6r29 |
1170875
to
2bb6dff
Compare
e034ddf
to
e3732f6
Compare
Awesome 🎉 Can you also update the test file? |
Done! |
Ahh, just realized I think I forgot to do a separate export for the Header props... still getting this workflow down! |
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.
This is great, @VanAnderson 🎉 Just left a few suggestions.
src/Header.tsx
Outdated
type StyledHeaderLinkProps = SystemCommonProps & SxProp & SystemTypographyProps & SystemBorderProps & {to?: boolean} | ||
|
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 type of to
should be History.LocationDescriptor
.
type StyledHeaderLinkProps = SystemCommonProps & SxProp & SystemTypographyProps & SystemBorderProps & {to?: boolean} | |
type StyledHeaderLinkProps = {to?: History.LocationDescriptor} & SystemCommonProps & SxProp & SystemTypographyProps & SystemBorderProps |
You'll need to the History
import at the top of the file:
import * as History from 'history'
src/Header.tsx
Outdated
const HeaderLink = styled.a.attrs(({to}: StyledHeaderLinkProps) => { | ||
const isReactRouter = typeof to === 'string' |
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.
When using attrs
, we've been passing the props type using the <>
notation:
const HeaderLink = styled.a.attrs(({to}: StyledHeaderLinkProps) => { | |
const isReactRouter = typeof to === 'string' | |
const HeaderLink = styled.a.attrs<StyledHeaderLinkProps>(({to}) => { | |
const isReactRouter = typeof to === 'string' |
Line 56-57 should look like this:
})<StyledHeaderLinkProps>`
font-weight: ${get('fontWeights.bold')};
Co-authored-by: Cole Bemis <colebemis@github.com>
Co-authored-by: Cole Bemis <colebemis@github.com>
Make sure changeset is uniform for Header component Co-authored-by: Cole Bemis <colebemis@github.com>
Co-authored-by: Cole Bemis <colebemis@github.com>
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.
👏
Migrate header component to TSX
Screenshots
No visual changes
Merge checklist
index.d.ts
) if necessaryTake a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.