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

Remove inactive classes from NavLink #7194

Closed
tim-phillips opened this issue Mar 14, 2020 · 14 comments
Closed

Remove inactive classes from NavLink #7194

tim-phillips opened this issue Mar 14, 2020 · 14 comments

Comments

@tim-phillips
Copy link

Using the NavLink component, I'd like to remove classes that should not be on an active link.

I'm using Tailwind and text-gray-200 does not override text-gray-800 when it is added to the element. I would like the class text-gray-800 to not be present while the link is active. Perhaps an inactiveClassName prop. I'd be happy to start a PR with some guidance.

What I've tried:

<NavLink
  to="/"
  className="px-3 py-2 text-gray-800"
  activeClassName="rounded-sm text-gray-200 bg-blue-gray-dark"
>
  Dashboard
</NavLink>

Could be:

<NavLink
  to="/"
  className="px-3 py-2"
  inactiveClassName="text-gray-800"
  activeClassName="rounded-sm text-gray-200 bg-blue-gray-dark"
>
  Dashboard
</NavLink>
@timdorr
Copy link
Member

timdorr commented Mar 14, 2020

You can just disable the !important declarations: https://tailwindcss.com/docs/configuration/#important

Then the text colors won't be overridden by their order in the CSS file.

@timdorr timdorr closed this as completed Mar 14, 2020
@tim-phillips
Copy link
Author

tim-phillips commented Mar 14, 2020

@timdorr, that setting is disabled by default. I tried false, true, and '#root' with no effect.

This has been discussed at tailwindlabs/tailwindcss#1010.

If you are trying to dynamically toggle between p-0 and p-4 you should always swap them out, toggle both, you shouldn't rely on css source order.


After doing some research, the class order defined on an HTML element has no effect, what matters is specificity. Since these two classes have the same specificity, the one that comes later in the stylesheet takes precedence.

https://stackoverflow.com/questions/12258596/class-overrule-when-two-classes-assigned-to-one-div/12258654

@garand
Copy link
Contributor

garand commented May 7, 2020

@timdorr Is this something you'd be willing to reconsider? I totally get adding extra bloat that doesn't make sense, but as I've been using tools like Tailwind CSS more recently as well it can become necessary to remove classes in "inactive" situations.

@timdorr
Copy link
Member

timdorr commented May 7, 2020

You can use @apply to create custom component classes from your Tailwind classes that better match the behaviors here.

@garand
Copy link
Contributor

garand commented May 7, 2020

@timdorr Yeah, I end up doing that usually but IMO it would be cleaner to only be applying the classes used on an element.

Appreciate the quick response!

@mjackson
Copy link
Member

It seems like we should probably change the default behavior of the activeClassName (and activeStyle) props to be exclusive instead of additive. That way you can be very explicit about what you want in both states. You may have to duplicate your "base" classnames and/or styles between the your normal and "active" props, but that would take care of this without introducing the need for another prop.

@mjackson mjackson reopened this Aug 26, 2021
@mjackson
Copy link
Member

... Then again, changing the behavior of the existing props would be a breaking change in v5. I think we should probably just go ahead and add an inactiveClassName prop as @tim-phillips originally suggested. We should probably also include a inactiveStyle prop for parity.

@bdbch
Copy link

bdbch commented Aug 27, 2021

I'm putting my + for going with the inactiveClassName option here. If we already have className and activeClassName - why not be super descriptive by also allowing the negative state of active (so inactive).

Would allow for finer control when using utility libraries.

@mjackson
Copy link
Member

@chaance Fixing this would be a great candidate for another v5 update next week.

@chaance
Copy link
Collaborator

chaance commented Aug 31, 2021

Just keeping everyone here in the loop, where I think we've landed is addressed in #7983. This solution is a bit lower level than simply adding inactiveClassName or inactiveStyle props, but it's a stronger base in v6 to handle either active or inactive styles with a single prop instead of splitting className or style into three separate props for each case.

The lovely thing with this approach is that, if you prefer an inactiveClassName prop, it's quite simple to create your own abstraction and use whatever API you'd prefer:

import { NavLink as BaseNavLink } from "react-router-dom";
import cx from "classnames"; // this can be whatever tool you want, obviously!

const NavLink = React.forwardRef(({ inactiveClassName, ...props }, ref) => {
  return (
    <BaseNavLink
      ref={ref}
      {...props}
      className={({ isActive }) => !isActive ? cx(props.className, inactiveClassName) : props.className}
    />
  );
});

@timdorr
Copy link
Member

timdorr commented Aug 31, 2021

Actually, a slightly more elegant way of doing it:

import { NavLink as BaseNavLink } from "react-router-dom";
import cx from "classnames"; // this can be whatever tool you want, obviously!

const NavLink = React.forwardRef(({ activeClassName, inactiveClassName, className, ...props }, ref) => {
  return (
    <BaseNavLink
      ref={ref}
      {...props}
      className={({ isActive }) => cx({ activeClassName: isActive, inactiveClassName: !isActive }, className)}
    />
  );
});

I like it! 👍

@chaance
Copy link
Collaborator

chaance commented Sep 1, 2021

Merged in #7983

@chaance chaance closed this as completed Sep 1, 2021
@tim-phillips
Copy link
Author

Nice solution! Thanks a ton!

@chaance
Copy link
Collaborator

chaance commented Sep 3, 2021

Just a note that we made a tiny tweak to the API before releasing, so isActive is a named object property in the function instead of the parameter itself. Just released in v6.0.0-beta.3!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants