-
-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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
react-router-config - renderRoutes(routes, otherProps = {}) #5137
Conversation
I'm happy to add an entry in |
Anything meta (version bumps, changelog entries, etc) don't belong in PRs, so don't worry about that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs some tests.
Also, I'd call it extraProps
instead. It's a tiny bit more descriptive.
Sure, I actually had to edit the PR description because I mistakenly thought I had called it extraProps. :-) I'll take a look at the existing tests and add tests as well |
Tests added, thanks for the feedback. Let me know if there's anything else I can do. |
Thanks! LGTM. I don't know if I should wait for a second +1 since it's still beta, but I'll merge it in a couple days if there are no objections. |
have you merged this feature ? |
@timdorr this is a useful feature that is hindering me from fully migrating to v4. I was about to begin working on the same PR. I would like to see this merged. Anything I can do to help ship this? |
If someone can resolve those merge conflicts, I can merge this in. Sorry, I forgot about this PR! |
I'm trying to merge, but wow talk about merge hell. The refactoring of the tests to Jest is making life interesting. I'm currently stuck on errors when running tests, related to resolving jest_cli from node_modules. Can anyone else do a pull from master and confirm that they can do I'm hitting troubles running the jest tests locally with npm 5.3 |
@timdorr ok, merged conflicts fixed. Please merge to master ASAP ;-) |
Thanks! |
@adamrneary Happy to see other people are finding the need for the same functionality! ;-) FWIW the renderRoutes code is so short, that I worked around it by adding a local copy like so:
|
When will this be released ? Thanks for your work ! |
@D34THWINGS looks like this was just released in 1.0.0-beta.4 @timdorr Any chance you're going to add this to the release notes page? |
@timdorr Oh, it is mentioned in the Kind of confusing since it's in a separate package... I also missed it because the title still says |
Add optional otherProps argument to react-router-config's renderRoutes function.
I'm working on migrating a rather complex project from react-router 3.x to 4.x, and I need the ability to pass props from a parent route's component to all child routes' components.
Adding the otherProps as an optional argument to renderRoutes allows me to do this...