Parameter feedback - #6 Better value normalization #4327
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ranbena
commented
Nov 1, 2019
| @@ -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)); | |||
There was a problem hiding this comment.
I wonder if there is a legitimate use case for multiple spaces in a parameter 😬
ranbena
commented
Nov 1, 2019
| return null; | ||
| } | ||
| const normalizedValue = toNumber(value); | ||
| return !isNaN(normalizedValue) ? normalizedValue : null; | ||
| return !isNaN(normalizedValue) ? normalizedValue : value; |
ranbena
commented
Nov 1, 2019
| return null; | ||
| } | ||
| return this.value; | ||
| } |
There was a problem hiding this comment.
I didn't see a reason why someone would use a parameter only containing whitespaces, so this looks good to me 👍.
ranbena
commented
Nov 1, 2019
| @@ -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; | |||
| } | |||
gabrieldutra
approved these changes
Nov 4, 2019
| value={normalize(value)} | ||
| onChange={val => this.onSelect(normalize(val))} | ||
| value={value} | ||
| onChange={val => this.onSelect(val)} |
| return null; | ||
| } | ||
| return this.value; | ||
| } |
There was a problem hiding this comment.
I didn't see a reason why someone would use a parameter only containing whitespaces, so this looks good to me 👍.
ranbena
changed the title
arikfr
approved these changes
Nov 13, 2019
| @@ -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)); | |||
There was a problem hiding this comment.
I wonder if there is a legitimate use case for multiple spaces in a parameter 😬
guidopetri
pushed a commit
that referenced
this pull request
Jul 21, 2023
* Parameter feedback - #4 Added in Dashboard params * Added cypress test * Moved to service * Parameter feedback - #5 Unsaved indication (#4322) * Parameter feedback - #5 Unsaved indication * Added ANGULAR_REMOVE_ME * Added cypress test * Fixed percy screenshot * Some code improvements * Parameter input feedback - #6 Better value normalization (#4327)
guidopetri
pushed a commit
that referenced
this pull request
Jul 21, 2023
* Parameter feedback - #3 Added in Widgets * Added cypress tests * Making sure widget-level param is selected * Parameter feedback - #4 Added in Dashboard params (#4321) * Parameter feedback - #4 Added in Dashboard params * Added cypress test * Moved to service * Parameter feedback - #5 Unsaved indication (#4322) * Parameter feedback - #5 Unsaved indication * Added ANGULAR_REMOVE_ME * Added cypress test * Fixed percy screenshot * Some code improvements * Parameter input feedback - #6 Better value normalization (#4327)
guidopetri
pushed a commit
that referenced
this pull request
Jul 21, 2023
* Parameter feedback - #2 Client errors in query page * Added cypress test * Fixed percy screenshot * Safer touched change * Parameter feedback - #3 Added in Widgets (#4320) * Parameter feedback - #3 Added in Widgets * Added cypress tests * Making sure widget-level param is selected * Parameter feedback - #4 Added in Dashboard params (#4321) * Parameter feedback - #4 Added in Dashboard params * Added cypress test * Moved to service * Parameter feedback - #5 Unsaved indication (#4322) * Parameter feedback - #5 Unsaved indication * Added ANGULAR_REMOVE_ME * Added cypress test * Fixed percy screenshot * Some code improvements * Parameter input feedback - #6 Better value normalization (#4327)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Text and Number parameters need some tweaking in terms of value normalization.
Text parameter
nullbut multiple spaces do not and should. Now trimming.dirtyindication. Now it won't.Number parameter
0to server. Now sendingnullinstead, displaying "Required parameter" error.Screenshots have been added inline in code.