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
Jkmarx/data set 2 tools param #1797
Conversation
…ry-platform into jkmarx/data-set-2-tools-param
…ry-platform into jkmarx/data-set-2-tools-param
Codecov Report
@@ Coverage Diff @@
## develop #1797 +/- ##
===========================================
+ Coverage 40.38% 40.46% +0.07%
===========================================
Files 386 389 +3
Lines 25334 25367 +33
Branches 1271 1271
===========================================
+ Hits 10232 10264 +32
- Misses 15102 15103 +1
Continue to review full report at Codecov.
|
}, | ||
templateUrl: ['$window', function ($window) { | ||
return $window.getStaticUrl('partials/tool-launch/partials/tool-params.html'); | ||
}] |
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.
Could you explain what's going on here, and why a plain string wouldn't work?
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 plain string would work here. A few weeks ago Ilya added a method to generate the static url based on django settings. So we use that method to generate the static urls as seen above.
class="panel-group" | ||
id="tool-info-display" | ||
role="tablist" | ||
aria-multiselectable="true"> |
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.
Are the aria-*
and role
tags just here for accessibility or something else? How would I know if I was using them correctly?
placeholder="{{ param.default_value }}" | ||
ng-model="$ctrl.paramsForm[param.uuid]" | ||
ng-maxlength="32" | ||
ng-pattern="/^\d+$/"> |
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.
Is this field allowed to be blank? Leading zeros probably won't cause problems? Do you want to accommodate negative values?
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.
Yes, there are default values for params. No idea, but this pattern can be changed once we have actual parameter specs.
placeholder="{{ param.default_value }}" | ||
ng-model="$ctrl.paramsForm[param.uuid]" | ||
ng-maxlength="32" | ||
ng-pattern="/^[0-9]+(\.[0-9]{1,2})?$/"> |
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.
should handle negatives? More than two decimal points? why [0-9] instead of \d?
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.
No idea, but this pattern can be changed once we have actual parameter specs.
@@ -1838,3 +1838,9 @@ ul.paragraph { | |||
float: left; | |||
} | |||
} | |||
|
|||
.form-validation-error { | |||
border: solid 1px rgba(246, 36, 0, 1); |
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.
Should we pull the color from a variable?
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.
consistency because the shadow needs a transparency.
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 validation regexes don't seem to allow negative values? Beyond that, there are obviously things I don't understand, but no obvious problems. See comments.
output_files: [], | ||
parameters: [ | ||
{ | ||
galaxy_workflow_step: 1, |
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.
Do we need this galaxy_workflow_step
in tests?
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.
I like to mock real-like data, but if we'd like to have a standard let's discuss.
) { | ||
var paramsService = toolParamsService; | ||
var vm = this; | ||
vm.isToolParamsCollapsed = false; // tracks the parameters panel |
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.
Thoughts on starting this collapsed?
I think if people are really dying to edit a tools params they will, otherwise they'll probably run them with the defaults at first.
tool_type: 'WORKFLOW', | ||
image_name: '', | ||
container_input_path: '', | ||
galaxy_workflow_id: 'c0279aab05812500', |
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.
Same forgalaxy_workflow_id
Ref: #1649