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

react-router-config - renderRoutes(routes, otherProps = {}) #5137

Merged
merged 5 commits into from Jul 23, 2017

Conversation

jharris4
Copy link
Contributor

@jharris4 jharris4 commented May 17, 2017

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...

@jharris4
Copy link
Contributor Author

I'm happy to add an entry in CHANGES.md, but I'm a bit of a n00b when it comes to lerna packages. Should I create a new changelog within the packages/react-router-config directory, or just edit the one in the root directory?

@jharris4 jharris4 changed the title react-router-config - renderRoutes(routes, extraProps = {}) react-router-config - renderRoutes(routes, otherProps = {}) May 17, 2017
@timdorr
Copy link
Member

timdorr commented May 17, 2017

Anything meta (version bumps, changelog entries, etc) don't belong in PRs, so don't worry about that.

Copy link
Member

@timdorr timdorr left a 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.

@jharris4
Copy link
Contributor Author

jharris4 commented May 17, 2017

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

@jharris4
Copy link
Contributor Author

Tests added, thanks for the feedback. Let me know if there's anything else I can do.

@timdorr
Copy link
Member

timdorr commented May 17, 2017

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.

@vilsad
Copy link

vilsad commented Jul 21, 2017

have you merged this feature ?

@sungwoncho
Copy link

@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?

@timdorr
Copy link
Member

timdorr commented Jul 22, 2017

If someone can resolve those merge conflicts, I can merge this in. Sorry, I forgot about this PR!

@jharris4
Copy link
Contributor Author

jharris4 commented Jul 22, 2017

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 npm install followed by npm test ?

I'm hitting troubles running the jest tests locally with npm 5.3

@jharris4
Copy link
Contributor Author

@timdorr ok, merged conflicts fixed.

Please merge to master ASAP ;-)

@timdorr
Copy link
Member

timdorr commented Jul 23, 2017

Thanks!

@timdorr timdorr merged commit 27b7d54 into remix-run:master Jul 23, 2017
@jharris4 jharris4 deleted the renderRoutes-extraProps branch July 23, 2017 18:26
@adamrneary
Copy link

@jharris4 thanks for this!
@timdorr i think this might be necessary to make animations work with react-router-config as you need to pass the location and a key to through to the child component. do you have a sense for when this might make a release?

Thanks so much!

@jharris4
Copy link
Contributor Author

@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:

// TODO - remove local version once react-router/pull/5137 is merged and released
// import { renderRoutes } from 'react-router-config';
import { Switch, Route } from 'react-router';
const renderRoutes = (routes, extraProps = {}) => routes ? (
  <Switch>
    {routes.map((route, i) => (
      <Route key={i} path={route.path} exact={route.exact} strict={route.strict} render={(props) => (
        <route.component {...props} {...extraProps} route={route}/>
      )}/>
    ))}
  </Switch>
) : null;

@D34THWINGS
Copy link

When will this be released ? Thanks for your work !

@jharris4
Copy link
Contributor Author

@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?

@jharris4
Copy link
Contributor Author

@timdorr Oh, it is mentioned in the react-router 4.2.0 notes.

Kind of confusing since it's in a separate package... I also missed it because the title still says otherProps instead of extraProps. lol

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

Successfully merging this pull request may close these issues.

None yet

6 participants