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

[Bug?]: navLink doesn't call onClick before navigating #9097

Open
1 task
razzeee opened this issue Aug 30, 2023 · 7 comments
Open
1 task

[Bug?]: navLink doesn't call onClick before navigating #9097

razzeee opened this issue Aug 30, 2023 · 7 comments
Labels
bug/needs-info More information is needed for reproduction help wanted p3 Low priority. The Core team will not prioritize this for now topic/router-&-navigation

Comments

@razzeee
Copy link
Contributor

razzeee commented Aug 30, 2023

What's not working?

NavLink should call onClick, before navigating to the route in to

I want to close a window, before leaving the page, basically.

I stumbled over this again #2346 (comment)

How do we reproduce the bug?

Here's a repro repo razzeee/redwood-navlink-repro

I would expect this to show up in the logs, if it worked razzeee/redwood-navlink-repro@main/web/src/components/Header/Header.tsx#L35

What's your environment? (If it applies)

No response

Are you interested in working on this?

  • I'm interested in working on this
@razzeee razzeee added the bug/needs-info More information is needed for reproduction label Aug 30, 2023
@jtoar
Copy link
Contributor

jtoar commented Sep 11, 2023

@razzeee thanks! I was playing the reproduction a bit. I still haven't gotten to the bottom of it, but if I move NavLink out of all the @headless/ui components, it works again. So there must be something at the intersection of those two that's causing the error.

@razzeee
Copy link
Contributor Author

razzeee commented Sep 12, 2023

Which reminds me of reading https://headlessui.com/react/menu#integrating-with-next-js which might be similar (or not at all)

@razzeee
Copy link
Contributor Author

razzeee commented Dec 1, 2023

Yeah, this tracks, the problem goes away when I do something like this

const MyNavLink = forwardRef(
  (props: any, ref: LegacyRef<HTMLAnchorElement>) => {
    const {
      href,
      children,
      to,
      activeClassName,
      matchSubPaths,
      className,
      ...rest
    } = props
    return (
      <NavLink
        href={href}
        to={to}
        className={className}
        activeClassName={activeClassName}
        matchSubPaths={matchSubPaths}
      >
        <a ref={ref} {...rest}>
          {children}
        </a>
      </NavLink>
    )
  }
)

There are some problems that seem to appear instead with the mouse behavior. But all in all, this probably means, that redwoods navLink isn't forwarding the ref correctly?

@Tobbe
Copy link
Member

Tobbe commented Dec 4, 2023

@razzeee Are you up for creating a PR that fixes the NavLinks?

@razzeee
Copy link
Contributor Author

razzeee commented Dec 4, 2023

@Tobbe I had a look and even tried to find the nextjs change/headlessui reference, but I can't find it. And I'm not seeing what's wrong.

My current non framework side implementation ends up with two nested tags and that breaks styling a bit.

@Tobbe
Copy link
Member

Tobbe commented Dec 8, 2023

Thanks for trying @razzeee
Unfortunately I don't have time to try to reproduce now, but it seems it's only a problem when NavLinks are used together with headless/ui, right?

I'll leave this open, but won't be able to prioritize right now. If more community members run into this and can help with diagnosis someone on the core team can pick this one up.

@Tobbe Tobbe added p3 Low priority. The Core team will not prioritize this for now help wanted topic/router-&-navigation labels Dec 8, 2023
@razzeee
Copy link
Contributor Author

razzeee commented Dec 8, 2023

I've had another look and I don't know why, but changing the nesting order seems to help.

Basically like tailwindlabs/headlessui#1965 (comment) hints at.

It's a bit more weird for disclosures and in parts not working like I would expect it, but it solves the inital problem for me/us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/needs-info More information is needed for reproduction help wanted p3 Low priority. The Core team will not prioritize this for now topic/router-&-navigation
Projects
None yet
Development

No branches or pull requests

3 participants