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

Add 'otherwise' wrapper method to route-segment #44

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

Add 'otherwise' wrapper method to route-segment #44

wants to merge 2 commits into from

Conversation

tminglei
Copy link

Hi @artch,

Your angular-route-segment not only added more features to angular-route, but also made the usage more elegant and simple!

This pull request is to add 'otherwise' wrapper method to route-segment.
I know you want not to make unnecessary adjustment for angular-route. But this little change will make user codes more consistent, since you already adjusted the 'when' semantic.

The route-segment 'otherwise' can be used as below:

$routeSegmentProvider.
    when('/', 's0').
    when('/1/a', 's1.a').
    when('/2', 's2').
    ...
    otherwise('/').
    ...

Can you help review and give your suggestions?


Thanks for your awesome job! :D

@artch
Copy link
Owner

artch commented Jun 23, 2014

Hm, wouldn't we probably want to assign otherwise to a segment name, rather than to an URL?

@tminglei
Copy link
Author

Well, assigning to an URL instead of a segment name, enable use to declare otherwise before or after other when declarations, instead of only after, since we obviously want to fetch redirectTo from it right now.

And assigning to a segment name seems not bring us any extra benefits. (Pls correct me if I was wrong.)

(p.s. the param route can also be a non-param function which return a URL. I just updated the comment.)

@qrb
Copy link

qrb commented Dec 22, 2014

Hi @artch @tminglei I'm also interested in an otherwise/catch-all route into the package.
Another possibility to achieve this could be to encapsulate the $routeSegmentProvider.when and
$routeProvider.otherwise into a single call which could be seen to simplify the scenario (?).

$routeSegmentProvider
  //normal route definitions
  .when('/', 'home')
  .when('/list/:category', 'list.category')

  //the catch-all route
  .else('/404', 'error')

The potential here is to define a segment as per usual and use the above syntax to specify the
catch-all route for that segment.

    $routeSegmentProvider.else = function(route, name) {
      this.when(route, name);
      $routeProvider.otherwise({redirectTo: route});
      return this;
    };

I'm happy to create a PR if you think this approach is valuable, otherwise it's a discussion point.

@artch I guess it really depends on what you want to see, but I tend to agree with @tminglei's
implementation if you wanted to introduce the $routeSegmentProvider.otherwise(). It would still be
preferable for the method to accept the 'params' argument as per $routeProvider where the user can
provide the segment name (not the route) to the redirectTo property.

@sivascse
Copy link

I got this error

$routeSegmentProvider.when(...).when(...).otherwise is not a function

where is the mistake?

@vdsabev
Copy link

vdsabev commented Feb 24, 2015

@sivascse there doesn't seem to be an otherwise function in $routeSegmentProvider yet.
This is what we need too - we'd like to preserve the 404 URL and instead of redirecting to a specific page, just render a template with a controller. The syntax would be identical to the one in $routeProvider:

$routeSegmentProvider.otherwise({
  controller: 'notFound',
  templateUrl: 'common/not-found.html'
});

@mcavallo
Copy link

I agree with @vdsabev. It would be great to display a template without having to redirect the user somewhere else, unless you decide to do so.

@andrenarchy
Copy link

What's the status here?

+1 for @vdsabev's proposal! Redirecting is not a clean solution...

@mischkl
Copy link

mischkl commented Jul 7, 2015

This seems like a simple patch that would be very useful. Can we get this accepted?

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.

8 participants