-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Parameter feedback - #6 Better value normalization #4327
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ import Input from 'antd/lib/input'; | |
import InputNumber from 'antd/lib/input-number'; | ||
import DateParameter from '@/components/dynamic-parameters/DateParameter'; | ||
import DateRangeParameter from '@/components/dynamic-parameters/DateRangeParameter'; | ||
import { isEqual } from 'lodash'; | ||
import { isEqual, trim } from 'lodash'; | ||
import { QueryBasedParameterInput } from './QueryBasedParameterInput'; | ||
|
||
import './ParameterValueInput.less'; | ||
|
@@ -59,7 +59,7 @@ class ParameterValueInput extends React.Component { | |
} | ||
|
||
onSelect = (value) => { | ||
const isDirty = !isEqual(value, this.props.value); | ||
const isDirty = !isEqual(trim(value), trim(this.props.value)); | ||
this.setState({ value, isDirty }); | ||
this.props.onSelect(value, isDirty); | ||
} | ||
|
@@ -140,13 +140,11 @@ class ParameterValueInput extends React.Component { | |
const { className } = this.props; | ||
const { value } = this.state; | ||
|
||
const normalize = val => (isNaN(val) ? undefined : val); | ||
|
||
return ( | ||
<InputNumber | ||
className={className} | ||
value={normalize(value)} | ||
onChange={val => this.onSelect(normalize(val))} | ||
value={value} | ||
onChange={val => this.onSelect(val)} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
/> | ||
); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import { toNumber, isNull } from 'lodash'; | ||
import { toNumber, trim } from 'lodash'; | ||
import { Parameter } from '.'; | ||
|
||
class NumberParameter extends Parameter { | ||
|
@@ -9,11 +9,11 @@ class NumberParameter extends Parameter { | |
|
||
// eslint-disable-next-line class-methods-use-this | ||
normalizeValue(value) { | ||
if (isNull(value)) { | ||
if (!trim(value)) { | ||
return null; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
const normalizedValue = toNumber(value); | ||
return !isNaN(normalizedValue) ? normalizedValue : null; | ||
return !isNaN(normalizedValue) ? normalizedValue : value; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import { toString, isEmpty } from 'lodash'; | ||
import { toString, isEmpty, trim } from 'lodash'; | ||
import { Parameter } from '.'; | ||
|
||
class TextParameter extends Parameter { | ||
|
@@ -15,6 +15,13 @@ class TextParameter extends Parameter { | |
} | ||
return normalizedValue; | ||
} | ||
|
||
getExecutionValue() { | ||
if (!trim(this.value)) { | ||
return null; | ||
} | ||
return this.value; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't see a reason why someone would use a parameter only containing whitespaces, so this looks good to me 👍. |
||
} | ||
|
||
export default TextParameter; |
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.
Before:
After:
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 wonder if there is a legitimate use case for multiple spaces in a parameter 😬