Skip to content

Conversation

@dmarcey
Copy link
Contributor

@dmarcey dmarcey commented Aug 30, 2019

I tried out the UnderlineNav and TabNav in a TypeScript app (create-react-app --typescript) and ran into some issues related to the types of the Links.

This PR cleans them up, as well as the propTypes on the UnderlineNav.Link and TabNav.Link

Merge checklist

  • Changed base branch to release branch
  • Add or update TypeScript definitions (index.d.ts) if necessary
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

@vercel
Copy link

vercel bot commented Aug 30, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@vercel vercel bot temporarily deployed to staging August 30, 2019 18:12 Inactive
Copy link
Contributor

@colebemis colebemis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome 💯 Thanks for updating this! Just left one comment about href.

index.d.ts Outdated

export interface UnderlineNavItemProps extends CommonProps {
export interface UnderlineNavLinkProps extends CommonProps {
href: string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
href: string
href?: string

Should href be optional?

Reach Router's Link component accepts a to prop instead of href. If someone does something like this:

<UnderlineNav.Link as={ReachLink} to="#" />

will it cause a TypeError?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I think it will. I was going for consistency with LinkProps where it is also required.

export interface LinkProps extends CommonProps, TypographyProps {
  href: string
  underline?: boolean
}

Do we want that to be optional as well, in case someone wanted to do:

<Link as={ReachLink} to="#" />
?

Copy link
Contributor

@colebemis colebemis Aug 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's a good idea.

@vercel vercel bot temporarily deployed to staging August 31, 2019 01:04 Inactive
Copy link
Contributor

@colebemis colebemis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! 👍 I wanna let @emplums take a look at this too before we merge it. She'll be back next week.

@emplums
Copy link

emplums commented Sep 9, 2019

This looks good to me, thanks @dmarcey!

@emplums emplums changed the base branch from master to release-13.5.0 September 9, 2019 16:21
@emplums emplums merged commit 2f451f4 into primer:release-13.5.0 Sep 9, 2019
@dmarcey dmarcey deleted the users/dmarcey/navTypings branch September 9, 2019 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants