-
Notifications
You must be signed in to change notification settings - Fork 598
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # 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.