-
Notifications
You must be signed in to change notification settings - Fork 14.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
Translate string to array for multi fields in getControlsState #5057
Translate string to array for multi fields in getControlsState #5057
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5057 +/- ##
=======================================
Coverage 77.53% 77.53%
=======================================
Files 44 44
Lines 8712 8712
=======================================
Hits 6755 6755
Misses 1957 1957 Continue to review full report at Codecov.
|
superset/assets/src/explore/store.js
Outdated
@@ -56,6 +56,10 @@ export function getControlsState(state, form_data) { | |||
delete control.mapStateToProps; | |||
} | |||
|
|||
if (control.multi && typeof formData[k] === 'string') { |
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.
nit: formData[k] = (control.multi && typeof formData[k] === 'string') ? [formData[k]] : formData[k]
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.
Would !Array.isArray(formData[k])
be better?
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 think we'd want to stick with typeof formData[k] === 'string'
because formData[k] might be undefined or null and we would want to leave it as null.
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.
Though I think it can be a int
/ float
too.
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.
Ah ok, updated it to formData[k] && !Array.isArray(formData[k])
.
🚢 |
f5badc8
to
d955c13
Compare
@mistercrunch @graceguo-supercat can one of you merge if |
Actually after thinking a bit more I'm thinking while this fixes the problem in the explore view, I don't think it fixes the issue in the dashboard view. |
@michellethomas maybe we need to refactor some sort of |
…e#5057) * Translate string to array for multi fields in getControlsState * Updating format to fit on one line
…e#5057) * Translate string to array for multi fields in getControlsState * Updating format to fit on one line
In the explore store.js, I'm adding logic that will handle a string value being passed to a multi control by making it an array. This would handle errors with existing slices when making a SelectControl multi.
Fixes #5011
@mistercrunch @graceguo-supercat