redwoodjs / redwood

The App Framework for Startups
https://redwoodjs.com
MIT License
17.28k stars 991 forks source link

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

Open razzeee opened 1 year ago

razzeee commented 1 year ago

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 https://github.com/redwoodjs/redwood/pull/2346#issuecomment-1348104484

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?

jtoar commented 1 year ago

@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 commented 1 year ago

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

razzeee commented 10 months ago

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 commented 10 months ago

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

razzeee commented 10 months ago

@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 commented 10 months ago

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.

razzeee commented 10 months ago

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

Basically like https://github.com/tailwindlabs/headlessui/discussions/1965#discussioncomment-4977167 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.