-
Notifications
You must be signed in to change notification settings - Fork 3
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
Ramenhog/version url param #16
Conversation
This is perfect for my needs!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor comment in the first file. Not sure whether I need to review use.js
.
interface/src/app/genesets/use.js
Outdated
$scope.chosenVersion = Versions.get(verParams); | ||
$location.search({ version: chosenVersionHash }); | ||
}; | ||
$scope.geneset.$promise.then(function(data) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to add a blank line between 280 and 281 to make it more readable.
interface/src/app/genesets/use.js
Outdated
var versionParam = $location.search().version; | ||
|
||
if (versionParam) { | ||
var matchingVersions = $scope.geneset.versions.filter(function( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The search of versionParam
doesn't have to scan the whole $scope.geneset.versions
. It will be done when one match is found. So it can be written like this:
if (versionParam) {
var found = $scope.geneset.versions.find(function(version) {
return version.ver_hash === versionParam;
});
if (found) {
initialVersion = versionParam;
}
}
...
Of course, if $scope.geneset.versions
includes only a few elements, then no big deal.
@@ -12,354 +12,409 @@ | |||
* The dependencies block here is also where component dependencies should be | |||
* specified, as shown below. | |||
*/ | |||
angular.module( 'tribe.genesets.use', [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you change anything else except the coding style in this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, I meant the other parts except showVersion()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, that's it 👍
Thanks, just addressed the concerns. Back to you :) @dongbohu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks.
Closes #13
The main changes to
use.js
are in theshowVersion()
function on line #271. The file was also reformatted with JSPrettier.