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

Standard <Link> tags causing two LinkContainers to be active at once. #242

Open
willbee28 opened this issue Oct 29, 2018 · 10 comments
Open

Comments

@willbee28
Copy link

willbee28 commented Oct 29, 2018

I am using <LinkContainer>s for my <NavDropdown.Item> s and have a sidenavigation with standard <Link> tags. Problem arises when I click on normal links, the linkcontainer follows and sets that linkcontainer to active but doesn't make the previous linkcontainer not active, so I end up with two active linkcontainers. Anyone have a fix? Code is here:

<LinkContainer exact to="/init-raw" >
<NavDropdown.Item onClick={() => this.setState({ isProcessOpen: false, isAnalyzeOpen: false })}>
Open Analysis </NavDropdown.Item>
</LinkContainer>
<LinkContainer exact to="/init-alt" >
<NavDropdown.Item onClick={() => this.setState({ isProcessOpen: false, isAnalyzeOpen: false })}> Open process </NavDropdown.Item>
</LinkContainer>

@manonthemoon42
Copy link

I also have the exact same issue.
A regular link would add active to the Nav.Link, but it doesn't remove the active from the last one.

@willbee28 , did you find any solutions for that issue?

@willbee28
Copy link
Author

willbee28 commented Feb 18, 2019 via email

@v12
Copy link
Member

v12 commented Apr 5, 2019

I'm afraid but this seems to be related to react-bootstrap as it relies on active property passed to <Nav.Link>, however, <LinkContainer> does handle className itself and doesn't do anything with active property. Thus, a workaround is to explicitly set active to false on each <Nav.Link>, leaving the rest to react-router-bootstrap to handle.

Or, as an option, you can create a helper component like this:

const RouterNavLink = ({ children, ...props }) => (
  <LinkContainer {...props}>
    <Nav.Link active={false}>
      {children}
    </Nav.Link>
  </LinkContainer>
)

However, I think we should consider adding a way to control the behaviour of an active prop passed to the component that is wrapped. @taion, @jquense, what do you think about this? I think it's required for hassle-free use of react-router and react-bootstrap, which we try to make friendly to each other by using react-router-bootstrap.

@taion
Copy link
Member

taion commented Apr 5, 2019

In previous versions we actually just injected the active prop to children: 2c28b9d#diff-7d608450e48ccf30094f19938db3948cL95

Could we just make that the behavior again?

@v12
Copy link
Member

v12 commented Apr 5, 2019

I think we can but I'd also consider adding a mechanism to control this behaviour as it's not just react-bootstrap this library is used for. Some of the use cases would require wrapped component to not have active property changed.

Maybe additional property (something like setActive), which is set to true by default? If truthy, then active property is explicitly set for a wrapped component, otherwise, do not touch it at all.

Will require another version bump if we implement it though as the default behaviour will change and new property will be set for a wrapped component...

@taion
Copy link
Member

taion commented Apr 5, 2019

so in other places i just have an activePropName prop on the container

the bigger question might be "what's a good default?" and i think passing down the active prop as the default behavior might be better than making assumptions about class names

@davidjb
Copy link

davidjb commented Mar 20, 2020

Just encountered this issue -- is there any further progress? @v12's workaround still works and is pretty clean.

@alekseykarpenko
Copy link

@v12 Thanks for your suggestion, it almost worked for me.

ℹ️ For those who will still fail with two different links being active even after providing @v12's workaround: make sure those links are always rerendered when changing your location from outside. In my code I used key with pathname passed into:

export const MyMenu = () => {
  const {pathname} = useLocation() // previously imported from 'react-router-dom'
  return (
    <ListGroup key={pathname}>
      <LinkContainer exact to="/path/to/first"><ListGroup.Item action active={false}>First Item</ListGroup.Item></LinkContainer>
      <LinkContainer exact to="/path/to/second"><ListGroup.Item action active={false}>Second Item</ListGroup.Item></LinkContainer>
      <LinkContainer exact to="/path/to/third"><ListGroup.Item action active={false}>Third Item</ListGroup.Item></LinkContainer>
    </ListGroup>
  );
};

Hope this may help someone in future.

@Lyubomyr
Copy link

@alekseykarpenko Thank you. Your solution is working just fine. But be aware that sometimes such a solution could be overkill and cause side effects. This means every time pathname will change (even some small part of it like id) ListGroup will re-render from scratch. And if you have some bigger logic inside ListGroup (e.g. list of the link depending on some data from the server) it could cause heavy re-rendering on each user click.

@einbulinda
Copy link

Just used NavLink from react-router-dom and passed className="nav-link" to it. Worked just fine.

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

No branches or pull requests

8 participants