Skip to content

[changed] activeRoute can render with props and children.#90

Merged
mjackson merged 1 commit intoremix-run:masterfrom
benjohnson:active-route
Jul 18, 2014
Merged

[changed] activeRoute can render with props and children.#90
mjackson merged 1 commit intoremix-run:masterfrom
benjohnson:active-route

Conversation

@benjohnson
Copy link
Copy Markdown
Contributor

Potentially fixes #59.

{this.props.activeRoute} is now {this.props.activeRoute(props, children)}.

mjackson added a commit that referenced this pull request Jul 18, 2014
[changed] activeRoute can render with props and children.
@mjackson mjackson merged commit 2d0fb47 into remix-run:master Jul 18, 2014
@garrensmith
Copy link
Copy Markdown

Is it possible to get documentation on this added to the readme.

@petehunt
Copy link
Copy Markdown
Contributor

FYI: as of react 0.11 you could do: <this.props.activeRoute />

@benjohnson
Copy link
Copy Markdown
Contributor Author

Yeah, that makes this so much better without needing to do hacky things with _owner. I'll throw together a branch that does this in the readme.

@benjohnson benjohnson deleted the active-route branch July 18, 2014 19:01
@luigy
Copy link
Copy Markdown
Contributor

luigy commented Jul 18, 2014

children can be more than one argument and are not getting passed down to the handler?
https://github.com/facebook/react/blob/master/src/core/ReactDescriptor.js#L142-L153

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

don't we need to route.props.handler.apply(null, [mergeProperties(props, addedProps)].concat(children)) to support this.props.activeRoute({}, child, child, child) ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

/facepalm. Yes we do. I've discovered that we also need to return a function when activeRoute is null, or in practice you need to do {this.props.activeRoute ? this.props.activeRoute() : null} which is annoying as hell. Both fixes on the way.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have no idea what people will use children for. It's a little confusing IMO.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I kind of agree, which is why I left it basically undocumented. But I think there's virtue in keeping consistent with the component API.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you build it, they will come.

Could use the same handler in two routes and have contextually different
children I guess?

On Fri, Jul 18, 2014 at 2:12 PM, Ben Johnson notifications@github.com
wrote:

In modules/components/Route.js:

 } else {
   props.activeRoute = null;
 }
  • childDescriptor = route.props.handler(props);
  • childHandler = function (props, addedProps, children) {
  •  return route.props.handler(mergeProperties(props, addedProps), children);
    

I kind of agree, which is why I left it basically undocumented. But I
think there's virtue in keeping consistent with the component API.


Reply to this email directly or view it on GitHub
https://github.com/rpflorence/react-nested-router/pull/90/files#r15131182
.

@ryanflorence ryanflorence mentioned this pull request Jul 24, 2014
@lock lock Bot locked as resolved and limited conversation to collaborators Jan 21, 2019
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.

Passing props to activeRoute

6 participants