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 $location service #2

Closed
ceoaliongroo opened this issue Apr 13, 2014 · 7 comments
Closed

Add $location service #2

ceoaliongroo opened this issue Apr 13, 2014 · 7 comments
Assignees
Milestone

Comments

@ceoaliongroo
Copy link

There are some benefits If use the module use $location instead window.location
http://docs.angularjs.org/guide/$location

@seriema
Copy link
Owner

seriema commented Apr 14, 2014

Yes, the reason we didn't use it was because at the time we need to call location (in .config()) it isn't available. If you know how to make it work, please let us know.

@seriema
Copy link
Owner

seriema commented Apr 15, 2014

We've changed things since then I remember now. It might be possible, will try it out! Hopefully it will allow unit-testing of that part. Busy creating demos atm.

@ceoaliongroo
Copy link
Author

@seriema Thanks,

I did a refactor of the code and add the unit test for your review.

Also, i'll create an issue #4 to extend the test and check or prepare the work with external data services.

@seriema
Copy link
Owner

seriema commented Apr 17, 2014

This issue can be the request for a pure $location refactor. If you do just the $location bit with the tests you have it would be awesome. Please sync down the latest code though, just did some big updates.

@seriema
Copy link
Owner

seriema commented Apr 17, 2014

Two things to think about is routing.

  1. Query parameters like apimock=true don't sit in window.location.search but window.location.hash. Need to verify that this works with $location.
  2. When using ngRoute it doesn't do a page refresh when adding ?apimock=true so the parameter isn't picked up. Need to make sure routes update the mock flag. See here for a sample where this breaks.

seriema added a commit that referenced this issue Apr 18, 2014
@ceoaliongroo
Copy link
Author

@seriema about:

  1. I understood the explanation, but still is difficult see the issue or the new feature that you propourse without a code example.

Could you create a fail spec in the test?
Thanks

  1. You could try to add reloadOnSearch=false in the route
    $routeProvider
      .when('/', {
        templateUrl: 'views/movies-table-list.html',
        controller: 'MoviesCtrl',
        reloadOnSearch: false
      })
      .otherwise({
        redirectTo: '/movies/list/'
      });
  });

Could you give access to the demo code?

All the best...

@seriema
Copy link
Owner

seriema commented Apr 26, 2014

  1. We don't need to worry about that anymore. It seems to be working with $location.

  2. Interesting! Two issues with that though. It would require ngRoute (we want to minimize dependencies) and it seems to be set on a per-url-basis (we need something generic).

The demo code is in the gh-pages-dev branch. Thinking about moving them here to a /docs folder or something.

@seriema seriema closed this as completed Apr 26, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants