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

added the notion of router hierarchy #82

Closed
wants to merge 14 commits into from

Conversation

MarkNijhof
Copy link

Now all routers are automatically added to a list and enabled the use of this.navigate('../other_route') which then takes the parent route to execute navigate on. Not tested but this should also enable this.navigate('../../other/route'). Works as well with [Link href="../other_route"]home[/Link]

Now all routers are automatically added to a list and enabled the use of this.navigate('../other_route') which then takes the parent route to execute navigae on. Not tested but this should also enable this.navigate('../../other/route')
ensure that there are no duplicate routers in the list
@MarkNijhof
Copy link
Author

I found a small bug, will fix tonight and add to this PR

@STRML
Copy link
Owner

STRML commented Sep 16, 2014

This is simple. I like it. Could you add some tests and squash the commits?

@MarkNijhof
Copy link
Author

I'll see how I can test this and will squash them when I have some tests. make test-local doesn't run for me. Do you depends on specific browsers? I am on Ubuntu with only Chrome

@STRML
Copy link
Owner

STRML commented Sep 16, 2014

make test-local should work if you npm install beforehand and make sure that port 3000 is unused on your machine. If you're still having problems, could you tell me what 'doesn't work' means?

@MarkNijhof
Copy link
Author

Ok that was simple, I had something running on port 3000 ;-) so what do you recommend creating some browser based tests to verify this functionality or unit tests

@STRML
Copy link
Owner

STRML commented Sep 16, 2014

Yeah, I would put it in browser.js - the other files are more specific. I would put it around the section where it starts testing contextual routers.

On Sep 16, 2014, at 4:54 PM, Mark Nijhof [email protected] wrote:

Ok that was simple, I had something running on port 3000 ;-) so what do you recommend creating some browser based tests to verify this functionality or unit tests


Reply to this email directly or view it on GitHub.

@MarkNijhof
Copy link
Author

Ok I added 6 tests 3 for history and 3 for hash api with contextual routers. I am having trouble rebasing the commits as there have been updates from master that I had applied as well in the middle of them. Any hints on how to do this properly?

Mark Nijhof and others added 8 commits September 17, 2014 00:25
Now all routers are automatically added to a list and enabled the use of this.navigate('../other_route') which then takes the parent route to execute navigae on. Not tested but this should also enable this.navigate('../../other/route')
added tests and fixed a bug
@MarkNijhof
Copy link
Author

I'll fix this differently :) new PR coming up

@MarkNijhof MarkNijhof closed this Sep 16, 2014
@STRML
Copy link
Owner

STRML commented Sep 16, 2014

Yeah the thing to do would be to rebase your branch on master - git rebase master should do it.

MarkNijhof pushed a commit to Frende/react-router-component that referenced this pull request Sep 16, 2014
Now all routers are automatically added to a list and enabled the use of this.navigate('../other_route') which then takes the parent route to execute navigate on. Not tested but this should also enable this.navigate('../../other/route'). Works as well with [Link href=../other_route]home[/Link]
@MarkNijhof
Copy link
Author

I added #84 sorry for any confusion

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

Successfully merging this pull request may close these issues.

2 participants