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

Allow NavLink to receive a children function #8164

Merged
merged 8 commits into from Dec 9, 2021
Merged

Allow NavLink to receive a children function #8164

merged 8 commits into from Dec 9, 2021

Conversation

sergiodxa
Copy link
Member

Right now the NavLink component has this useful className and style function support to change the styles if the link is active.

<NavLink to="." className={({ isActive }) => isActive ? activeClassName : inactiveClassName}>Content</NavLink>
<NavLink to="." style={({ isActive }) => isActive ? activeStyles : inactiveStyles}>Content</NavLink>

This is really cool, but one use case it doesn't support, but it's really easy to add support for, is what happens if inside the NavLink there are two, or more, elements and they need to change styles based on the isActive state, right now it's not really possible to detect that, specially when using utility CSS.

This PR adds support for that, which was also requested on the discussion #8080, copying the example from that discussion, now this is possible:

<NavLink to="path">
  {({isActive}) => (
    <>
      <Icon className={isActive ? 'text-blue-500' : 'text-gray-500'} />
      <span className={isActive ? 'text-blue-900' : 'text-gray-700'}>Link</span>
    </>
  )}
</NavLink>

I also updated the docs/api.md to mention and show an example of this and the tests of the NavLink component to test this works.

This allows NavLink to work this way
```jsx
<NavLink to="/home" className={({ isActive }) => isActive ? activeClassNames : inactiveClassNames}>
  {({ isActive }) => isActive ? <ActiveNavLinkContent /> : <InactiveNavLinkContent />}
</NavLink>
```
Comment on lines -399 to +405
style={({ isActive }) =>
isActive ? activeStyle : undefined
className={({ isActive }) =>
isActive ? activeClassName : undefined
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this example to use className instead of style, so the API docs of NavLink now can have one example of style, className and children as functions.

@mcansh
Copy link
Collaborator

mcansh commented Dec 7, 2021

this is technically already possible using a custom (Nav)Link component with the useResolvedPath and useMatch hooks - https://reactrouter.com/docs/en/v6/examples/custom-link. here's an example using an icon https://stackblitz.com/edit/github-kzress?file=src/App.tsx

though I personally am not opposed to the changes here

@ryanflorence ryanflorence added the accepted Accepted the changes, waiting for tests/CLA/etc. label Dec 9, 2021
@ryanflorence
Copy link
Member

Let's do it, @sergiodxa, I fixed the conflicts, you want to sign the contributors.yml in this PR and I'll merge it?

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Dec 9, 2021

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@sergiodxa
Copy link
Member Author

Let's do it, @sergiodxa, I fixed the conflicts, you want to sign the contributors.yml in this PR and I'll merge it?

Done! :shipit:

@timdorr
Copy link
Member

timdorr commented Dec 9, 2021

Fixed up the formatting for you, but it looks like there are a couple of test failures, unfortunately.

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Dec 9, 2021

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@sergiodxa
Copy link
Member Author

@timdorr I'm pretty sure it should be fixed now, could you run the workflow again? 🙏

@ryanflorence ryanflorence merged commit 75f8e2d into remix-run:dev Dec 9, 2021
brophdawg11 pushed a commit that referenced this pull request Mar 27, 2024
…f request time (#8164)

Co-authored-by: Mark Dalgleish <mark.john.dalgleish@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Accepted the changes, waiting for tests/CLA/etc. CLA Signed react-router-dom
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants