Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

$router.navigate to component #256

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

gotenxds
Copy link

I was thinking it was quite odd that we cant navigate by component name and only by url, so I added the functionality.

You can now do this:

AppController.$routeConfig = [
  { path: '/',              redirectTo: '/welcome' },
  { path: '/welcome',       component: 'welcome' },
  { path: '/goodbye',       component: 'goodbye' }
];
function AppController($router) {
  $router.navigate('welcome');
}

You can still use the same function to navigate by url.

Currently the function tries by url, if none found it tries by component name.
Maybe this should be separated ?

$router.navigateToUrl('/welcome');
$router.navigateToComponent('welcome');

Your thoughts ?

Review on Reviewable

@btford
Copy link
Contributor

btford commented Apr 26, 2015

Needs corresponding tests, but otherwise looks alright!

@gotenxds
Copy link
Author

I already added tests, If you think I missed some use cases let me know and I'll add them.

@btford
Copy link
Contributor

btford commented Apr 26, 2015

I don't see any in the commits in this PR. Can you update your branch, maybe?

@gotenxds
Copy link
Author

Amm this is wired, I guess I forgot to push that commit, I'll do that when I get home and update.

@gotenxds
Copy link
Author

Hi, added a single test, couldn't think of any more uses cases that are not already being taken care of.

The build seems to be failing but that's not because of my commit (so awesome?).
Anyway, let me know what you think.

@gotenxds
Copy link
Author

gotenxds commented May 4, 2015

Rebased to master, build passes now.

{ path: '/', component: 'one' }
]);

$router.navigate('/');
Copy link

Choose a reason for hiding this comment

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

Shouldn't this be testing $router.navigate('one');?

Copy link
Author

Choose a reason for hiding this comment

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

indeed it should, I have fallen victim to the monster of copy&paste, Thanks :)

@sdornan
Copy link

sdornan commented May 8, 2015

👍

@hannahhoward
Copy link

awesome work! hope it gets merged.

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.

4 participants