Skip to content
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

Fix query based dropdown not adding quote marks correctly #4186

Merged
merged 6 commits into from
Oct 6, 2019

Conversation

gabrieldutra
Copy link
Member

@gabrieldutra gabrieldutra commented Sep 25, 2019

What type of PR is this? (check all applicable)

  • Bug Fix

Description

This is not perfect yet, I still wanna explore a few more options without having the Dropdown Options in the Parameter structure context (that's the only difference from the regular Dropdown code - what handles if the value is valid is the Input).

Related Tickets & Documents

Fixes #4184

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

--

@gabrieldutra gabrieldutra self-assigned this Sep 25, 2019
Copy link
Member Author

@gabrieldutra gabrieldutra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue seemed to be caused due to some inconsistencies in the structure (e.g: Not having an array in the value when it should). The difference from the regular Dropdown parameter is that its options are known from the structure of the parameter, so it's possible to normalize the value from the setValue method.

Considering that, I now just make sure it's always an array from the Parameter structure and improved the value handling in the input. @ranbena @kravets-levko lmk if you have other ideas (imo ideally we should have access to the options in the parameter structure, but I don't think an async request in there is a good thing 😅).

}
}

setValue(value) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now using a value from state + this method to make sure no invalid values will go to the Select component.

@@ -387,7 +393,8 @@ export class Parameter {
if (has(query, key)) {
if (this.multiValuesOptions) {
try {
this.setValue(JSON.parse(query[key]));
const valueFromJson = JSON.parse(query[key]);
this.setValue(isArray(valueFromJson) ? valueFromJson : query[key]);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out numbers are also parsed and converted 😅, so this makes sure we don't convert it unless it's an array.

@@ -253,6 +253,12 @@ export class Parameter {
}
}

if (this.type === 'query' && !isEmptyValue(value)) {
if (this.multiValuesOptions && !isArray(value) && value !== null) {
value = [value];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QueryBasedDropdown with multi-values should always hold array values in its structure.

@gabrieldutra gabrieldutra marked this pull request as ready for review September 27, 2019 00:05
@arikfr arikfr merged commit d8a0af1 into master Oct 6, 2019
@arikfr arikfr deleted the fix-multi-query-dropdown branch October 6, 2019 08:35
@arikfr
Copy link
Member

arikfr commented Oct 6, 2019

🐛

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants