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

NavLink has no location property #4910

Closed
fb55 opened this Issue Apr 4, 2017 · 4 comments

Comments

2 participants
@fb55
Copy link
Contributor

fb55 commented Apr 4, 2017

Version

4.0.0

Expected Behavior

All other components (except Link, which couldn't do anything with it) have a location property that is incredibly useful when using react-router in combination with Mobx. NavLink is the exception here. Is there a reason it wasn't implemented?

@pshrmn

This comment has been minimized.

Copy link
Collaborator

pshrmn commented Apr 4, 2017

I'm not sure what a <NavLink location> would be used for. Can you give an example? A <NavLink> naturally uses the location from its nearest parent <Route> to determine if it is "active".

@fb55

This comment has been minimized.

Copy link
Contributor

fb55 commented Apr 4, 2017

The location would be passed on to the child Route, to push an update inside a pure component.

We're wrapping all react-router components to listen to a mobx location store that triggers updates as soon as the location changes. Otherwise only router components that are direct children of the Router would receive updates.

@pshrmn

This comment has been minimized.

Copy link
Collaborator

pshrmn commented Apr 5, 2017

Right. So you're running into the blocked updates issue, which this guide aims to help with. You should only have to worry about passing the location to your observer'd components, because those are the ones that are blocking location change triggered updates.

@fb55

This comment has been minimized.

Copy link
Contributor

fb55 commented Apr 5, 2017

Nice guide :)

Still, it is way too easy to miss one spot where state needs to be updated, especially several layers down. Having the navigation components listen for updates works remarkably well and means that the rest of the application does not need to be concerned with navigation.

I've opened #4918 that implements the necessary change.

timdorr added a commit that referenced this issue Apr 11, 2017

Add location property to NavLink (#4918)
* Add location property to NavLink

fixes #4910

* Add test case for location property on NavLink
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment