radix-ui / primitives

Radix Primitives is an open-source UI component library for building high-quality, accessible design systems and web apps. Maintained by @workos.
https://radix-ui.com/primitives
MIT License
15.51k stars 791 forks source link

[NavigationMenu] Menu should close when navigating to a `Link` #1301

Closed haniotis closed 2 years ago

haniotis commented 2 years ago

Feature request

Overview

Add open state to NavigationMenu.Root so we can override opening/closing states for use with external lib (e.g. Framer Motion).

Same functionality as found in Collapsible, DropdownMenu, Dialog, etc.

Edit: There's also no way of closing the menu when navigation between pages in a Next.js app and the menu stays open (located in global layout outside page content)

Thank you!

weskor commented 2 years ago

I see that this is closed, so i'm not sure if I can reopen it or leave a comment here. Currently, i'm trying to implement this in a Next.js app and want the navigation to close when the user clicks on a link. @haniotis did you find a solution for this in the end?

andy-hook commented 2 years ago

Hi @weskor , just to clarify, is your issue that the menu isn't closing when navigating client side to another route in your app? i.e. the "open" state of the menu is persisting on route change?

weskor commented 2 years ago

Hi @andy-hook, yes that seems to be the case. I could not find anything in the documentation regarding closing. Not sure if this the intended behavior and that I have to implement something for this.

andy-hook commented 2 years ago

I could not find anything in the documentation regarding closing. Not sure if this the intended behavior and that I have to implement something for this.

@weskor This should be being handled for you out of the box to be honest so seems like an oversight on our part.

We'll raise a fix soon but in the meantime you can control the value yourself and set it to an empty string in order to close the menu manually

https://codesandbox.io/s/distracted-gwen-domugj?file=/pages/_app.js

Hope this helps 🙏

weskor commented 2 years ago

Hi @andy-hook, thank you so much 🙏 Really love what Radix is doing and thanks for the quick reaction.

andy-hook commented 2 years ago

Add open state to NavigationMenu.Root so we can override opening/closing states for use with external lib

@haniotis Just to clarify on this point, the functionality for managing the "open" state does exist but rather than a boolean value per Collapsible, DropdownMenu, Dialog this menu uses a value prop which controls the currently active Item. Out of the box we generate these but if you need to control the currently active item you can assign your own to each item.

https://www.radix-ui.com/docs/primitives/components/navigation-menu#item

Happy to provide an example of animating this with framer if you're still having issues.

haniotis commented 2 years ago

@andy-hook Yup! That's what I ended up figuring out therefore closed the issue.

Wouldn't mind an example with framer-motion though - that would be greatly appreciated

samburgers commented 2 years ago

thanks for this! i implemented a slight change to @andy-hook example, hooking into route changes rather than click events - meaning it should catch things like browser fwd/back buttons and keyboard shortcuts etc too

  const [value, setValue] = React.useState("");
  const { asPath } = useRouter();

  React.useEffect(() => {
    setValue("");
  }, [asPath]);

https://codesandbox.io/s/mystifying-germain-mlb900?file=/pages/_app.js

shawnmclean commented 1 year ago

I'm using the Sheet component (seems to be built on dialog).

I have this same issue, so the recommended way is to watch for a router change and then set the component to close?

andy-hook commented 1 year ago

so the recommended way is to watch for a router change and then set the component to close

Yes

shawnmclean commented 1 year ago

I hope I can get some help here for Sheet behavior:

export function UserNav() {
  const [menuOpen, setMenuOpen] = useState(false);

  const pathname = usePathname();
  const searchParams = useSearchParams();

  useEffect(() => {
    setMenuOpen(false);
  }, [pathname, searchParams]);

  return (
    <Sheet open={menuOpen}>
      <SheetTrigger asChild onClick={() => setMenuOpen(true)}>
        <Button>
             Avatar
        </Button>
      </SheetTrigger>
      <SheetContent>
          <Button asChild>
            <Link href="/settings">Settings</Link>
          </Button>
      </SheetContent>
    </Sheet>
  );
}

This works somewhat, but when I'm already on the /settings page, it doesn't change the route, so it doesn't close.

I'm trying to understand your PR change but not quite sure whats going on.

Appreciate any help explaining how to transfer your code to this scenario.

Thank you!

andy-hook commented 1 year ago

@shawnmclean can you provide a codesandbox showing your existing implementation? would help us provide some guidance.

maurer2 commented 3 months ago

Hello, I believe it might be possible to avoid useEffect here by using the pathname that is returned from the usePathname hook as a key for the navigation component. This would mean that the Navigation child component that contains all the radix components would be reset whenever the pathname changes. The approach is described in the new React docs: https://react.dev/learn/you-might-not-need-an-effect#resetting-all-state-when-a-prop-changes

It could look something like this:

NavigationWrapper component

'use client'

import { usePathname } from 'next/navigation';
import Navigation from './navigation';

export function NavigationWrapper() {
  const pathname = usePathname();

  return (
    <Navigation key={pathname} />
  )
}

Cheers

andy-hook commented 3 months ago

I believe it might be possible to avoid useEffect here by using the pathname that is returned from the usePathname hook as a key for the navigation component.

Also a good option providing you don't have additional state that needs to be persisted.