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

[RFC]: NavLink should not merge classes #10296

Closed
1 task done
Tobbe opened this issue Mar 26, 2024 · 1 comment · Fixed by #10342
Closed
1 task done

[RFC]: NavLink should not merge classes #10296

Tobbe opened this issue Mar 26, 2024 · 1 comment · Fixed by #10342

Comments

@Tobbe
Copy link
Member

Tobbe commented Mar 26, 2024

Summary

<NavLink> currently does a very naive merging of className and activeClassName on the active page.

const theClassName = [className, matchInfo.match && activeClassName]
  .filter(Boolean)
  .join(' ')

Given this <NavLink>

<NavLink
  to={routes.air()}
  className="inline-block rounded-t-lg border-b-2 border-transparent p-4 hover:border-gray-300 hover:text-gray-600 dark:hover:text-gray-300"
  activeClassName="active inline-block rounded-t-lg border-b-2 border-blue-600 p-4 text-blue-600 dark:border-blue-500 dark:text-blue-500"
>
  Air
</NavLink>

The css classes it ends up with in the browser are: inline-block rounded-t-lg border-b-2 border-transparent p-4 hover:border-gray-300 hover:text-gray-600 dark:hover:text-gray-300 active inline-block rounded-t-lg border-b-2 border-blue-600 p-4 text-blue-600 dark:border-blue-500 dark:text-blue-500

The intention is for the border to be blue for the active page, but it actually ends up being transparent. It has both the border-transparent and border-blue-600 classes, but for some reason border-transparent wins here. Probably because of the order they're defined in the TW sources (but I didn't check, so not 100% sure).

Motivation

Developers should have full control over what classes gets applied to the link

Detailed proposal

The simplest way to give the developer full control here is to not try to merge the classes, but rather just use className OR activeClassName, not both. Yes, more repetition is needed, but it does provide more control over the styling.

Another option is to provide a prop to control the behavior. But I think that adds extra complexity that I don't want.

Are you interested in working on this?

  • I'm interested in working on this
@cannikin
Copy link
Member

cannikin commented Mar 26, 2024

This is the behavior that we have in the form helpers (className and errorClassName are NOT merged and one replaces the other). I would have thought we’d do the same thing here but I guess not!

I agree, although you may have to repeat some classes, it’s the only way to be sure you have full control over the styles in both active and in-active states.

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.

2 participants