-
Notifications
You must be signed in to change notification settings - Fork 300
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
HPCC-14453 Preflight upgrade #9165
HPCC-14453 Preflight upgrade #9165
Conversation
@GordonSmith Please review. |
https://track.hpccsystems.com/browse/HPCC-14453 |
return retVal; | ||
}, | ||
|
||
getColumns: function (params) { |
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 this be called "setColumns" or "setGridColumns"?
var clean = finalColumns[index].replace(/([~!@#$%^&*()_+=`{}\[\]\|\\:;'<>,.\/? ])+/g, '').replace(/^(-)+|(-)+$/g,''); | ||
if (clean === 'Condition') { | ||
lang.mixin(dynamicColumns, { | ||
[clean] : { |
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.
What does this syntax do (I have not seen it before)?
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.
This creates dynamic properties with the value from clean which in turn is 'Condition' 'State' 'RoxieCluster'
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 didn't know you could do that with this syntax!
if (params) { | ||
var finalColumns = params.Columns.Item; | ||
for (index in finalColumns) { | ||
var clean = finalColumns[index].replace(/([~!@#$%^&*()_+=`{}\[\]\|\\:;'<>,.\/? ])+/g, '').replace(/^(-)+|(-)+$/g,''); |
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 removing HTML formatting?
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.
Since the response returns "Column Name" I am removing any spaces and or special characters in it so that they come back like so: "ColumnName"
} | ||
} | ||
}); | ||
} if (clean === 'State') { |
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 "if" should be on a new line.
WsMachine.GetMachineInfo ({ | ||
request: { | ||
[MachineInformationClean]: filter[MachineInformationClean], | ||
[MachineInformationCount]: 1000, |
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 syntax correct?
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 verified with @wangkx this count seems to increase based on selections and is subjective.
WsMachine.GetTargetClusterInfo ({ | ||
request: { | ||
[TargetClustersClean]: filter[TargetClustersClean], | ||
[TargetClusterCount]: 1000, |
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 syntax correct?
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 verified with @wangkx this count seems to increase based on selections and is subjective.
}, | ||
|
||
createLabelAndElement: function (id, label, element, placeholder, value) { | ||
var context = this; |
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 declare var control;
here and then assign values to it in the switch statement
Some questions. |
b316a16
to
52dd2a3
Compare
@GordonSmith updated. |
@miguelvazq Needs rebasing for a clean merge |
52dd2a3
to
684916f
Compare
@richardkchapman rebased. |
@@ -1 +1 @@ | |||
Subproject commit 468db35af8875930d424395808ddc9523c57c6f2 | |||
Subproject commit d7bbad34db39a51f209c6fadd07c9ec3a0bb86b7 |
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 not be in this PR
|
||
table.miniSelect span { | ||
width:20px; | ||
} |
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 for an edit in place control?
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.
This is for the selects that are smaller in nature such as "%" and or "MB"
var control = element; | ||
switch (element) { | ||
case "CheckBox": | ||
var control = new CheckBox ({ |
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.
var control should be declared outside of the switch for clarity.
}); | ||
break; | ||
case "TextBox": | ||
var control = new ValidationTextBox ({ |
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.
var control should be declared outside of the switch for clarity.
}); | ||
break; | ||
case "Select": | ||
var control = new Select ({ |
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.
var control should be declared outside of the switch for clarity.
}); | ||
break; | ||
case "SelectMini": | ||
var control = new Select ({ |
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.
var control should be declared outside of the switch for clarity.
style: "vertical-align:middle;padding:2px 0 2px 5px;" | ||
}) | ||
) | ||
control.placeAt(this.id + id); |
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.
control is used outside of swicth - so var control should be declared before the switch statement, in case none of the branches are called.
Add pre flight submission form into new ECL Watch. Signed-off by: Miguel Vazquez <miguel.vazquez@lexisnexis.com>
684916f
to
37283ae
Compare
@GordonSmith changes made after latest comments |
Automated Smoketest HPCC Stop: OK |
@GordonSmith Please re-review/approve |
Good to go. |
Add pre flight submission form into new ECL Watch.
Signed-off by: Miguel Vazquez miguel.vazquez@lexisnexis.com