Skip to content
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

Next.js Link component incompatible with Hero CTAs #63

Open
rezrah opened this issue Aug 11, 2022 · 3 comments
Open

Next.js Link component incompatible with Hero CTAs #63

rezrah opened this issue Aug 11, 2022 · 3 comments
Assignees
Labels
brand good first issue Good for newcomers

Comments

@rezrah
Copy link
Collaborator

rezrah commented Aug 11, 2022

Problem statement

Next.js encourage using the Link component declaratively to wrap any <a> tags.

E.g.

<Link href="/about">
  <a>About Us</a>
</Link>

The Hero doesn't use a composable API, which makes composition of Next.js link impossible.

<Hero
  heading="..."
  description="..."
  primaryAction={{
    text: 'Primary action',
    href: '#',
  }}
  secondaryAction={{
    text: 'Secondary action',
    href: '#',
  }}
  align="..."
/>

Suggestion

Rewrite the API for consistency with other composable API's.

<Hero align="...">
  <Hero.Heading></Hero.Heading>
  <Hero.Description></Hero.Description>
  <Hero.Description></Hero.Description>
  <Link href="#">
    <Hero.PrimaryAction></Hero.PrimaryAction>
  </Link>
  <Link href="#">
    <Hero.SecondaryAction></Hero.SecondaryAction>
  </Link>
</Hero>

References

Next.js Link component docs

@rezrah rezrah added the brand label Aug 11, 2022
@lesliecdubs lesliecdubs added the good first issue Good for newcomers label Aug 11, 2022
@rfearing
Copy link
Contributor

FYI: I spoke with Max and we don't recall any issues with Next/link. (Context: I mentioned we had some speedbumps with Next's link, but what I was thinking of is actually Next's image component.

@rezrah rezrah changed the title Next.js Link component not compatible with Hero CTAs Next.js Link component incompatible with Hero CTAs Sep 12, 2022
@stefankp
Copy link

Wondering if we should remove the forced vertical padding on Hero 🤔 None of the other components have that.

@rfearing
Copy link
Contributor

Wondering if we should remove the forced vertical padding on Hero 🤔 None of the other components have that.

That makes sense to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
brand good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

5 participants