NavLink breaks className and styles DOM API

[andrecasal]

What version of Remix are you using?

1.16.1

Are all your remix dependencies & dev-dependencies using the same version?

• [X] Yes

Steps to Reproduce

The NavLink component accepts both a string and a function in its className and style attributes. This is a convenient API to quickly set up navigation-aware styles. However, this violates the DOM API that only accepts a string and you might get in trouble when using composition, for example.

Here's an example of when you might get in trouble using (Epic Stack recommended) Radix UI's NavigationMenu component and composing it with Remix's NavLink:

const MenuItem = ({ to, children }: { to: string; children?: ReactNode) => (
	<NavigationMenu.Item>
		<NavigationMenu.Link asChild>
			<NavLink to={to} className={({isActive}) => `${isActive ? 'active-classes' : 'inactive-classes'} other-classes`}>
				{children}
			</NavLink>
		</NavigationMenu.Link>
	</NavigationMenu.Item>
)

Because Radix expects className to be a string, it will render class="({ isActive }) => `${isActive ? "active-classes" : "inactive-classes"} other-classes` active" onto the DOM instead of class="active-classes other-classes.

Solutions

• Add a thin wrapper around NavLink. Here's a codesanbox by @benoitgrelard.

• If you're using Tailwind, use the NavLink's aria-current attribute:

const MenuItem = ({ to, children }: { to: string; children?: ReactNode) => (
	<NavigationMenu.Item>
		<NavigationMenu.Link asChild>
			<NavLink to={to} className="aria-[current]:underline">
				{children}
			</NavLink>
		</NavigationMenu.Link>
	</NavigationMenu.Item>
)

Suggestions

Add a note to the documentation that the NavLink breaks the DOM API standard and provide these workarounds 👆 to fix it.

Related issues: https://github.com/remix-run/remix/issues/1966 https://github.com/radix-ui/primitives/issues/1158 https://github.com/radix-ui/primitives/issues/2157

Expected Behavior

Expected documentation to note this.

Actual Behavior

Function is printed onto the DOM.

[giovannibenussi]

I'm not used that much to Radix, but after a while and a quick test on my computer I think I got the issue.

In the end you can reduce the issue to the code:

<NavigationMenu.Link asChild>
  <NavLink
    to={to}
    className={({ isActive }) =>
      `${isActive ? "text-green-500" : "text-blue-500"}`
    }
  >
    {children}
  </NavLink>
</NavigationMenu.Link>

The NavigationMenu.Link asChild expects an anchor tag and merges its props. That's why it's converting the className function to a string. Below is an example of the output of the previous code:

<a data-radix-collection-item="" class="({ isActive }) => `${isActive ? &quot;text-green-500&quot; : &quot;text-blue-500&quot;} other-classes`" href="/login/hi">Hello</a>

I think the solution you proposed about being explicit about this in the docs is ok. Although in the current docs it says "...works like a normal className, but you can also pass it a function to customize the classNames applied...". Maybe say something like "Allows to provide a css class string to the component. It also extends the standard API to support a function to customize...".

[hjonasson]

Well researched and argued bug report 🔥. In my opinion, this is a Radix issue and not a Remix issue. The issue seems to come from this react-slot within Radix here which looks like a component that introduces many issues. It is used to make a copy of the children you pass in but makes many assumptions about those children (notice they insist your handlers start with on<Capital letter>. That is common practice yes but I should be able to name my handler handleClick, handler, or somethingElse without knowing my component library this intricately).

I would like to see the link in the Remix docs about className as @andrecasal suggested. 👍

[a-eid]

wrap your navlink is a react fragment,

   <Tooltip.Trigger asChild>
    <>
      <NavLink
        to={to}
        className={({ isActive }) =>
          "block rounded-md p-4 transition-colors hover:bg-gray-100 " + (isActive && "bg-gray-100")
        }
      >
        <img src={src} className="h-6 w-6 text-gray-100" />
      </NavLink>
    </>
  </Tooltip.Trigger>;

edit:

a fragment breaks Tooltip.Trigger, replace it with a div

   <Tooltip.Trigger asChild>
    <div>
      <NavLink
        to={to}
        className={({ isActive }) =>
          "block rounded-md p-4 transition-colors hover:bg-gray-100 " + (isActive && "bg-gray-100")
        }
      >
        <img src={src} className="h-6 w-6 text-gray-100" />
      </NavLink>
    </div>
  </Tooltip.Trigger>;

[bradymwilliams]

If you're using Tailwind, use the NavLink's aria-current attribute:

Thank you for this

[AsemK]

any plans to fix this?