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

[NavigationMenu] Expose position of active <NavigationMenu.Item /> #1462

Open
janhoogeveen opened this issue Jun 10, 2022 · 40 comments
Open
Labels
Package: react/navigation-menu Type: Enhancement Small enhancement to existing primitive/feature

Comments

@janhoogeveen
Copy link

janhoogeveen commented Jun 10, 2022

Feature request

Can we expose the position of the active <NavigationMenu.Item />, and maybe it's bounding box so we can position the <NavigationMenu.Viewport /> directly below the active item?

It could be exposed through CSS custom properties as --radix-navigation-menu-indicator-position, which should work for both horizontal and vertical menu's.

Even better would be a hook or render prop so we can wait until the indicator knows its position before we show the <NavigationMenu.Viewport />

Overview

The NavigationMenu works really well if your foldout content is wider than your list of navigation menu items.

Screenshot_10_06_2022__13_02

However, if you add (much) more NavigationMenu.Item components the layout will start to look awkward.

image

Example on codesandbox: https://02xv7f.csb.app/

Who does this impact? Who is this for?

Everyone who wants to use NavigationMenu with a wide list of items.

@janhoogeveen
Copy link
Author

janhoogeveen commented Jun 10, 2022

I realize another solution is to wrap <NavigationMenu.Viewport /> in <NavigationMenu.Indicator /> but I'm not sure if that's frowned upon? 🤔

One significant downside of this approach is that you get a short window of time where the CSS custom property indicating the position of the indicator is not set yet, so the ViewportPosition jumps around.

const StyledIndicatorWithArrow = React.forwardRef((props, forwardedRef) => (
  <StyledIndicator {...props} ref={forwardedRef}>
    <StyledArrow />
    <ViewportPosition>
      <NavigationMenuViewport />
    </ViewportPosition>
  </StyledIndicator>
));

Example: https://codesandbox.io/s/cocky-heisenberg-biwho8?file=/App.js

@andy-hook
Copy link
Collaborator

Hey @janhoogeveen , I agree this is a bit of a pain point at the moment, providing some way to track the content against the active trigger would be very useful for folks 👍

I realize another solution is to wrap <NavigationMenu.Viewport /> in <NavigationMenu.Indicator /> but I'm not sure if that's frowned upon?

As you noted, the indicator wasn’t designed for this purpose. In addition to the visual glitch there is also an accessibility problem as aria-hidden is set on the indicator part by default so it would effectively hide all menu content from screen readers.

Thanks for raising, I'm going to mark it for improvement. In the meantime it is possible to implement your own tracking logic fairly easily depending on your design requirements, here is a (really) rough example of how it could work with a centrally aligned navigation:

https://codesandbox.io/s/navigation-menu-track-position-fhh9so

@andy-hook andy-hook added Type: Enhancement Small enhancement to existing primitive/feature Package: react/navigation-menu labels Jun 14, 2022
@andy-hook andy-hook changed the title [Navigation] Expose position of active <NavigationMenu.Item /> [NavigationMenu] Expose position of active <NavigationMenu.Item /> Jun 14, 2022
@janhoogeveen
Copy link
Author

Hey @andy-hook, great to hear! Curious to see what kind of API you're going to end up! 👍🏻

I really appreciate your example, it looks solid and easy enough to use in the meantime!
Thanks so much

@elisehein
Copy link

@andy-hook Thanks for the example!

I'll be looking forward to this feature as well.

Would you mind explaining why you set the list ref with setState rather than useRef in your example?

@andy-hook
Copy link
Collaborator

Would you mind explaining why you set the list ref with setState rather than useRef in your example?

@elisehein It ensures the ref callbacks are updated with the list node when it’s available, there are likely more efficient ways to build this but I wanted to provide a quick example to show that this is possible in user code with some additional measurement.

Another approach might be to get and sync the currently active trigger via effect and update the positioning accordingly:

https://codesandbox.io/s/navigation-menu-track-position-effect-sync-q20tjd?file=/src/App.js

It depends on your design and the types of animation effects you’re trying to achieve, this example is dependent on central alignment so a different approach or more generic implementation might be necessary depending on the need.

It also doesn't account for responsive changes in element dimensions, for that you'd need to observe for element resize etc.

Hope this helps!

@jailsonpaca
Copy link

any news in this issue?

@christian-reichart
Copy link

What's the best way to center the content below the item, but keeping it "contained" in the viewport, so the content will stick to the left or right screen edge, instead of going off-screen?

I'm trying to build a Main navigation menu, it is aligned to the right inside the header. For the right-most navigation items the content goes off-screen currently.

Thanks for any hints

@heymath
Copy link

heymath commented Feb 3, 2023

Hello 👋

Is it OK to use a Dropdown in a NavigationMenu? Or can we use a Menubar as Navigation?
I guess both answers are "no", but in cas I am wrong 😄

Because what I want is a submenu displayed like a dropdown just below the NavigationMenu.Item (see screenshot below).

I don't want a common view port to display links on the middle of the Navigation ... the example for NavigationMenu looks nice, but it does not look like the most common design for a Navigation. In my opinion, most navigations look like the MenuBar actually or a NavigationMenu + Dropdowns.

And sticking to NavigationMenu seems to add a lot of complexity to position it below the NavigationMenu.Item and handle screen edges... It is the kind of things I am expecting Radix to handle for me. What Dropdown seems to do already by the way.

Moving from Reakit v1 to RadixUI was easy until I started migrating the last component of my todo list (MainNav), and now I am hard stuck on my migration Pull Request 😅

Thanks for any help! 🙏

Screenshot of desired design

Capture d’écran du 2023-02-03 15-45-44

@andy-hook
Copy link
Collaborator

Or can we use a Menubar as Navigation?

Nope, as you say, Menubar shouldn't be used as navigation

@heymath how many levels deep is your nav? is it only one level per your screenshot?

@heymath
Copy link

heymath commented Feb 3, 2023

Or can we use a Menubar as Navigation?

Nope, as you say, Menubar shouldn't be used as navigation

@heymath how many levels deep is your nav? is it only one level per your screenshot?

Yes it is only one level 😉

Thanks!

@andy-hook
Copy link
Collaborator

andy-hook commented Feb 3, 2023

@heymath I don't think it's much complexity to use the NavigationMenu and position the content as you require, you are correct that it doesn't provide collision handling but it's typically not necessary in the majority of cases.

Website navigation typically sits at the head of a page by convention so collisions are unlikely, and as horizontal width becomes limited it's typical to switch to a separate collapsed drawer / accordion style anyway.

Here's a simple example:
https://codesandbox.io/s/simple-navigation-menu-em1qrc?file=/src/App.js

Using NavigationMenu like this provides the appropriate accessibility patterns and keyboard handling specifically designed for website navigation. A list of dropdown menus is an option, but that primitive was not intended for that purpose and unless you really need the collision handling I would recommend the above instead.

Hope this helps.

@heymath
Copy link

heymath commented Feb 3, 2023

@andy-hook well actually you can see on the screenshot that the "Plus" dropdown is at the end of the nav, and the nav is large (very often it that takes the whole screen width or more on small screens with a horizontal scroll).
So we have that case where the drop-down is out of screen and creates a horizontal scroll etc.
I am on mobile right now, I will have a look at the code you provided layer, to see if it can be adapted easily or not.

Anyway, I have to say that I like very much migrating other components from Reakit to Radix. It was clear and simple.
But this NavigationMenu component was the opposite. A lot of things not handle by default and a default behavior that does not seems natural/the most common case to me. I was skeptical if it was the good one or if I had to merge it with other components etc. 😅

By the way I am very new to this, I only spent one day exploring this migration, maybe I don't see how things can be done yet and it is not that difficult to achieve what we want...

Thanks again for your answers!

@heymath
Copy link

heymath commented Feb 7, 2023

@andy-hook maybe there is another way to achieve it, but staying close to the current Reakit implementation, and my work in progress of the migration, I am stuck at this:

https://codesandbox.io/s/simple-navigation-menu-forked-v5d6qp?file=/src/App.js

My problem here is that Radix does not auto position the Content to be aligned with the Trigger or does not Provide any thing to help to do it (unless I missed it). And I do not see how to achieve it 🤔

Is it normal to not have this kind of feature included in NavigationMenu while it is in DropdownMenu & Menubar components?

@joepetrillo
Copy link

Any updates on this?

@jailsonpaca
Copy link

up

@ReangeloJ
Copy link

Ah facing the same issue, thought initially it was a bug in the shadcn/ui package.

@jailsonpaca
Copy link

still not resolved?

@thejprice
Copy link

I was going to implement this navigation menu into our website, but ran into this very issue. Seems like it would be a nice feature to include? The example conveniently uses only 3 menu items to hide this issue. 🤔

@Josevictor2
Copy link

UP

@sgup
Copy link

sgup commented Oct 5, 2023

Would really appreciate an easy prop to achieve this

@alexander-densley
Copy link

I've been using shadcn/ui alot recently, and I have to say this navigation menu seems like a poorly created component. Very little customizability, and no prop to get rid of the open on hover. Seems like a lot of functionality that is pretty standard in well developed ui libraries are not present in this radix nav menu comp. I'm currently using the sheet component now to create a my nav menu since it allows me to do what I need.

@recallwei
Copy link

+1

1 similar comment
@Fahl-Design
Copy link

+1

@mspangl5
Copy link

Any updates on this? It is shocking to me that a component library that is designed to be style-less has something this unorthodox built into it by default without a simple way of disabling it. Basically every website has a navbar and most of them are wider than their example in the docs.

@FalaseNoemdek
Copy link

How to make the menubar active color when clicked

@JoshuaBellew
Copy link

Any updates on this? Would love to be able to override and choose the position of the dropdowns :)

@damyco
Copy link

damyco commented Dec 12, 2023

Ran into the same issue as everyone else, would love to have some props to control some of the behaviour.

@abdurezakfarah
Copy link

+1

@akram19-setif
Copy link

+1

@SalahAdDin
Copy link

Up!

@madebyisaacr
Copy link

Any update on this? NavigationMenu doesn't work at all with smaller menus because of this issue. Thanks!

Screen.Recording.2024-02-16.at.1.52.24.AM.mov

@codermuyi
Copy link

This issue has been open for almost 2 years. Dev team, please look into it. We keep facing this issue.

@jhgeluk
Copy link

jhgeluk commented Apr 2, 2024

Please implement this

@jonasborsch
Copy link

jonasborsch commented Apr 3, 2024

Stuck with the same issue. I'm going to write a workaround for now.
To me this seems like a normal use case that should be considered. Would love to see this fixed in a future update.

@SalahAdDin
Copy link

Stuck with the same issue. I'm going to write a workaround for now. To me this seems like a normal use case that should be considered. Would love to see this fixed in a future update.

can you share the workaround?

Did people here open a discussion on the discord server?

@nirmpate
Copy link

I ended up just replacing the menu items that need a dropdown with a Popover component. And it worked for me.

@geudrik
Copy link

geudrik commented Jul 1, 2024

Also having the same positioning issue - not all menu bars are short, or even center aligned...

@SalahAdDin
Copy link

I ended up replacing the menu items needing a dropdown with a Popover component. And it worked for me.

could you make a tutorial?

@danieljpgo
Copy link

I realize another solution is to wrap <NavigationMenu.Viewport /> in <NavigationMenu.Indicator /> but I'm not sure if that's frowned upon? 🤔

One significant downside of this approach is that you get a short window of time where the CSS custom property indicating the position of the indicator is not set yet, so the ViewportPosition jumps around.

const StyledIndicatorWithArrow = React.forwardRef((props, forwardedRef) => (
  <StyledIndicator {...props} ref={forwardedRef}>
    <StyledArrow />
    <ViewportPosition>
      <NavigationMenuViewport />
    </ViewportPosition>
  </StyledIndicator>
));

Example: https://codesandbox.io/s/cocky-heisenberg-biwho8?file=/App.js

This solution is great for quickly solve the problem; however, it still has an issue: the first time it opens, it will render the Viewport in the initial position.

I found a simple solution that is working for my case:

<NavigationMenu.Root>
  <NavigationMenu.List>
    {...}
    <NavigationMenu.Indicator aria-hidden={false}>
      <NavigationMenu.Viewport />
    </NavigationMenu.Indicator>
  </NavigationMenu.List>
  <NavigationMenu.Viewport className="hidden" /> // display: none 
</NavigationMenu.Root>

or

<NavigationMenu.Root>
  <NavigationMenu.List>
    {...}
  </NavigationMenu.List>
   <NavigationMenu.Indicator  aria-hidden={false}>
      <NavigationMenu.Viewport />
   </NavigationMenu.Indicator>
   <NavigationMenu.Viewport className="hidden" /> // display: none 
</NavigationMenu.Root>

Initially, the Viewport tries to be rendered inside Root, so if we create two instances, with one in Root and the other inside Indicator and prevent the instance in Root from being rendered, we will not have the initial flash of the Viewport trying to render in Root.

This is a crazy hack for a quick solution. Ideally, we would have an exported variable at a higher level so that we have manual control of the viewport position, but until then, this works.

@embyth
Copy link

embyth commented Aug 8, 2024

Here is my solution using MutationObserver in NavigationMenu.Root to track open state on menu items and calculate their positions within navigation container:

import { forwardRef, useEffect, useRef } from 'react';
import * as NavigationMenuPrimitive from '@radix-ui/react-navigation-menu';
import { cn } from '~/lib/utils';
import { NavigationMenuViewport } from './NavigationMenuViewport';

export function mergeReferences<T = unknown>(
  references: Array<React.MutableRefObject<T> | React.LegacyRef<T>>,
): React.RefCallback<T> {
  return (value) => {
    for (const reference of references) {
      if (typeof reference === 'function') {
        reference(value);
      } else if (reference != null) {
        (reference as React.MutableRefObject<T | null>).current = value;
      }
    }
  };
}

export const NavigationMenu = forwardRef<
  React.ElementRef<typeof NavigationMenuPrimitive.Root>,
  React.ComponentPropsWithoutRef<typeof NavigationMenuPrimitive.Root>
>(({ className, children, ...props }, reference) => {
  const containerReference = useRef<HTMLElement>(null);

  useEffect(() => {
    const container = containerReference.current;

    if (!container) return;

    const updatePosition = (item: HTMLElement) => {
      const menuItemRect = item.getBoundingClientRect();
      const containerRect = container.getBoundingClientRect();

      const position = {
        top: menuItemRect.top - containerRect.top,
        left: menuItemRect.left - containerRect.left,
      };

      container.style.setProperty('--radix-navigation-menu-item-active-top', `${position.top}px`);
      container.style.setProperty('--radix-navigation-menu-item-active-left', `${position.left}px`);
    };

    const mutationCallback = (mutationsList: MutationRecord[]) => {
      for (const mutation of mutationsList) {
        if (
          mutation.type === 'attributes' &&
          mutation.attributeName === 'data-state' &&
          mutation.target instanceof HTMLElement &&
          mutation.target.hasAttribute('aria-expanded') &&
          mutation.target.dataset.state === 'open'
        ) {
          updatePosition(mutation.target);
        }
      }
    };

    const observer = new MutationObserver(mutationCallback);

    observer.observe(container, {
      childList: true,
      attributes: true,
      subtree: true,
    });

    return () => {
      observer.disconnect();
    };
  }, []);

  return (
    <NavigationMenuPrimitive.Root
      ref={mergeReferences([reference, containerReference])}
      className={cn('relative z-10 flex max-w-max flex-1 items-center justify-center', className)}
      {...props}
    >
      {children}
      <NavigationMenuViewport />
    </NavigationMenuPrimitive.Root>
  );
});
NavigationMenu.displayName = NavigationMenuPrimitive.Root.displayName;

And we can access --radix-navigation-menu-item-active-left variable from NavigationMenu.Viewport:

export const NavigationMenuViewport = forwardRef<
  React.ElementRef<typeof NavigationMenuPrimitive.Viewport>,
  React.ComponentPropsWithoutRef<typeof NavigationMenuPrimitive.Viewport>
>(({ className, ...props }, reference) => (
  <div
    className={cn(
      'absolute left-0 top-full flex translate-x-[var(--radix-navigation-menu-item-active-left)] transform justify-center transition-transform duration-100',
    )}
  >
    <NavigationMenuPrimitive.Viewport
      className={cn(
        'origin-top-center bg-popover text-popover-foreground relative mt-1.5 h-[var(--radix-navigation-menu-viewport-height)] w-full overflow-hidden rounded-md border shadow-lg data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-90 md:w-[var(--radix-navigation-menu-viewport-width)]',
        className,
      )}
      ref={reference}
      {...props}
    />
  </div>
));
NavigationMenuViewport.displayName = NavigationMenuPrimitive.Viewport.displayName;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: react/navigation-menu Type: Enhancement Small enhancement to existing primitive/feature
Projects
None yet
Development

No branches or pull requests