Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Change router so route can be a callback function #2834

Closed
wants to merge 1 commit into from
Closed

Change router so route can be a callback function #2834

wants to merge 1 commit into from

Conversation

richardcrichardc
Copy link
Contributor

Thinking about feedback from #2571 which I submitted a few weeks ago, here is another idea for making routing extensible. It has the benefit of allowing you to use the existing router for routes it is good at.

The premise is that no router is likely to meet everyone's requirements, so make it plugable. It turns out it already is. You can register another service with the name '$router'. However, I would argue that that is a lot of work when the current router almost does what you want. It turns out other routers exist, but I have not found one that meets my requirements.

This pull requests allows you to pass a callback function as a route. Below is an example. It is a simplification of what I need to do, in fact cat is a path like x/y/ or x/y/123 so more parsing is done. Excuse the coffescript:

categoryRoute = (params, search) ->
  if params.cat.match(/^\d+$/)
    return { templateUrl: '/t/item.html', controller: ItemCtrl, resolve: ItemCtrl.resolve }
  else  
    return { templateUrl: '/t/category.html', controller: CategoryCtrl, resolve: CategoryCtrl.resolve }

...
...

$routeProvider.when '/category/*cat', categoryRoute

Another feature not shown here is that params and search can be updated by the route function - though I have not tested this for search.

Any feedback would be appreciated. Let me know if this is likely to be merged, then I can add documentation and do more thorough testing.

It would also be helpful to get some idea whether updating the router is a priority of the core developers of angular and if so what they see as key priorities.

@richardcrichardc richardcrichardc mentioned this pull request May 31, 2013
Closed
@petebacondarwin
Copy link
Contributor

PR Checklist (Minor Feature)

  • Contributor signed CLA now or in the past (if you just signed, leave a comment here with your real name for cross reference)
  • Feature improves existing core functionality
  • API is compatible with existing Angular apis and relevant standards (if applicable)
  • PR doesn't contain a breaking change
  • PR contains unit tests
  • PR contains e2e tests (if suitable)
  • PR contains documentation update
  • PR passes all tests on Travis (sanity)
  • PR passes all tests on ci.angularjs.org (cross-browser compatibility)
  • PR is rebased against recent master
  • PR is squashed into one commit per logical change
  • PR's commit messages are descriptive and allows us to autogenerate release notes (required commit message format)
  • All changes requested in review have been implemented

@richardcrichardc
Copy link
Contributor Author

Re: PR Checklist

Any feedback would be appreciated. Let me know if this is likely to be merged, then I can add documentation and do more thorough testing.

This is not ready to merge. At this stage I am just soliciting feedback. I'm not trying to be obtuse or anything, but there is not much point spending the time tidying this up if it is not going to be merged, otherwise it's just busy work.

I am more than happy to have a discussion about the proposed functionality.

@petebacondarwin
Copy link
Contributor

OK but you could sign the CLA anyway for good measure :-)

@richardcrichardc
Copy link
Contributor Author

Yes. I signed that electronically about three months ago, when I submitted
#2571 #2571. Maybe it is
under the email address richardc@richardc.net. Let me know if you still
cannot find it and I will fill the form in again.

On 18 July 2013 01:40, Pete Bacon Darwin notifications@github.com wrote:

OK but you could sign the CLA anyway for good measure :-)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2834#issuecomment-21113150
.

@jeffbcross
Copy link
Contributor

@richardcrichardc: @chirayuk and I did a quick review and like the functionality. Could you flesh this out further and add documentation and tests so we can review for merging? And no coffeescript in examples please! :)

@richardcrichardc
Copy link
Contributor Author

OK. I've merged the changes with the master from this morning, added some docs and unit tests. It's on a new branch here: https://github.com/richardcrichardc/angular.js/tree/routeFunction2. I suspect if I merge the changes with my original branch it will show all changes since 1.1.4. here.

@richardcrichardc richardcrichardc mentioned this pull request Jul 26, 2013
Closed
@richardcrichardc
Copy link
Contributor Author

A fine example of how not to use github. Updated patch here: #3359

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@richardcrichardc @petebacondarwin @jeffbcross