Handle images with no user properly #147
Conversation
| if($scope.maxTagsPage > $scope.tagsCurrentPage){ | ||
| var nextPageNumber = $scope.tagsCurrentPage + 1; | ||
| return '/repository/'+$scope.repository+'/'+ $scope.tagsPerPage +'/' +nextPageNumber; | ||
| return '/repository/'+$scope.repository+'?tagsPerPage='+ $scope.tagsPerPage +'&tagPage=' +nextPageNumber; |
There was a problem hiding this comment.
I'm not sure that tagsPerPage can be a parameter. In line 63 of app/app.js this route is defined as
when('/repository/:repositoryUser/:repositoryName/:tagsPerPage?/:tagPage?', {As you can see, here it is part of the path and not a parameter.
Correct me, if I'm mistaken.
There was a problem hiding this comment.
Apart from named parameters,
$routeParamsalso parses the query string, if present. So, if your URL contains a query string like?key1=value1, the key/value pair is added to$routeParams.
Also see the example at the bottom of https://docs.angularjs.org/api/ngRoute/service/$routeParams:
// Given:
// URL: http://server.com/index.html#/Chapter/1/Section/2?search=moby
// Route: /Chapter/:chapterId/Section/:sectionId
//
// Then
$routeParams ==> {chapterId:'1', sectionId:'2', search:'moby'}Also I think you'll find it works if you checkout my branch and run it. Especially if you use Chrome and install the ng-inspector for AngularJS Chrome extension; with that extension installed, you can go to any page rendered by Angular and click the little "A" button in the toolbar and get a pane that allows you to browse the values of all the variables in $scope.
I did forget to remove :tagsPerPage? from the routes though. I can update that.
There was a problem hiding this comment.
@kwk: Have I convinced you that tagsPerPage can be a query string parameter? If so, I think it also makes sense to do the same with reposPerPage...
in `app/app.spec.js`, so I can work on getting this case working.
Check if `$scope.repositoryUser` is defined and if it's not, set `$scope.repository` simply to `$scope.repositoryName`.
instead of putting `tagsPerPage` in the route part of the URL, which makes it difficult to distinguish between `repositoryName` and `tagsPerPage`, because the routes are too similar.
instead of putting `tagsPerPage` in the route part of the URL, which makes it difficult to distinguish between `repositoryName` and `tagsPerPage`, because the routes are too similar.
from `RepositoryListController` to `TagController`. I think it makes more sense to read `defaultTagsPerPage` in `TagController` than to do it in `RepositoryListController` and have to pass it in URLs.
Updated the unit tests to page `tagsPerPage` as a query string parameter rather than in the route.
an make them work by casting string to an int with `parseInt`.
for case when there is no `repositoryUser`.
when image has no `repositoryUser`.
when image has no `repositoryUser`.
I think it looks better.
msabramo
force-pushed
the
no_repositoryUser_make_it_work
branch
from
545aa65 to
e997230
Compare
|
@kwk: How does this look? |
|
Awesome! |
2 participants
This makes it so that images that don't have a user (e.g.:
ubunturather thangliderlabs/registrator) work properly.There have been several PRs I think that tried to tackle this (e.g.: #100 , #104 & #97), but this one has tests (see 9cb8ea1)! 😄
Note that this changes the routing in a fairly significant way, because it switches to using query string variables for things like
tagsPerPageandtagPage. The reason is that this helps to make the routes less ambiguous. For example, with the old routing, if you had a URI like/repository/ubuntu/10(the intent is that this means no user, image name isubuntu, 10 tags per page), the routing could easily get confused and match a route like/repository/:repositoryUser/:repositoryNameand think thatrepositoryUserisubuntuandrepositoryNameis10. Switching to query string variables removes this ambiguity, because?tagsPerPage=10makes it clear that the10is the number of tags per page and not the repository name.Fixes #121
Closes #97, #100, #104