Skip to content

[added] <DefaultRoute> component #200

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

Merged
merged 2 commits into from
Aug 14, 2014
Merged

[added] <DefaultRoute> component #200

merged 2 commits into from
Aug 14, 2014

Conversation

mjackson
Copy link
Member

Also, changed behavior of routes with no name, path, or children so
they act as default routes. is essentially just sugar.

Fixes #164
Fixes #193

* Only one such route may be used at any given level in the
* route hierarchy.
*/
function DefaultRoute(props) {
Copy link
Member

Choose a reason for hiding this comment

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

we need @spicyj to show us how to do this properly before the next release of react, apparently we are cheating here and need to make components, not just functions that can trick JSX, but for now :shipit:

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you by chance mean that something like <Route to='somewhere' handler={ShellHandler} defaultHandler={DefaultHandler} /> would be more correct instead? Definitely easer then doing checks to make sure there is only one DefaultRoute child.

Copy link
Member

Choose a reason for hiding this comment

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

No, I mean how do we make a real component out of <DefaultRoute/> instead of just a function that jsx thinks is a component.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, thanks for the clarification.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can imagine that <DefaultRoute handler={Foo} /> will be compiled into {type: DefaultRoute, props: {handler: Foo}}. I am pretty sure that RegisterRoute will just have to understand what DefaultRoute is.

@ryanflorence
Copy link
Member

crap, I messed this all up by merging 6d1ae95

@ryanflorence
Copy link
Member

@mjackson is it intentional that you now have to specify path="/" on root routes?

<Routes>
  <Route handler={App}>
    <DefaultRoute handler={Index}/>
    ...
  </Route>
</Routes>

Doesn't work anymore. I thought maybe I needed to use DefaultRoute instead:

<Routes>
  <DefaultRoute handler={App}>
    <DefaultRoute handler={Index}/>
    ...
  </DefaultRoute>
</Routes>

But that doesn't work either.

@ryanflorence
Copy link
Member

oh, I see now that it was intended for this to fix #193 ... anyway, I rebased with master, this branch no longer has conflicts.

@mjackson
Copy link
Member Author

is it intentional that you now have to specify path="/" on root routes?

No, that's not intentional if it's true, but it shouldn't be. You should still be able to leave the path off as usual. Root routes get path="/" and nested routes get the path of their parent.

@mjackson
Copy link
Member Author

6d1ae95 isn't really a fix AFAICT. It only satisfies his use case. This branch contains a real fix for #193.

@ryanflorence
Copy link
Member

yeah, I already merged that, but have rebased this commit with master and removed it, all is well.

@ryanflorence
Copy link
Member

No, that's not intentional if it's true, but it shouldn't be. You should still be able to leave the path off as usual. Root routes get path="/" and nested routes get the path of their parent.

Doesn't work. I had to add this: https://github.com/rackt/react-router/pull/200/files#diff-cfa924f8d5ffb353e9d877f0273257efR51

@mjackson
Copy link
Member Author

Doesn't work.

ok, it does now. I've updated the examples and fixed the bug. I'll let you merge when you think it's ready.

@mjackson mjackson mentioned this pull request Aug 14, 2014
Also, changed behavior of routes with no name, path, or children so
they act as default routes. <DefaultRoute> is essentially just sugar.

Fixes #164
Fixes #193
ryanflorence added a commit that referenced this pull request Aug 14, 2014
[added] <DefaultRoute> component
@ryanflorence ryanflorence merged commit 364b211 into master Aug 14, 2014
@ryanflorence ryanflorence deleted the default-route branch August 14, 2014 23:00
@adregan
Copy link

adregan commented Aug 15, 2014

FWIW,I found that the path="/" on root Routes was only necessary once I redirected the root (default handler work around).

@mjackson mjackson mentioned this pull request Aug 26, 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.

Allow partial declaration of routes Add "default route" handler
5 participants