-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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(explore): Value undefined and Unhashable type errors #22207
Conversation
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.
One minor first pass comment, will test in an hour or so
@@ -276,21 +277,36 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => { | |||
>(state => state.explore.controlsTransferred); | |||
|
|||
useEffect(() => { | |||
const removeDatasourceWarningFromControl = ( |
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: Could we define this outside in a useCallback
?
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.
We could, but that wouldn't bring much value I think since we won't use it in any other place. I guess it's a style preference
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.
You're right, I've tended to err on the side of just spraying useCallback
everywhere, but I agree it's not really needed in trivial cases like this where 1) creating it is dirt cheap 2) it's not being used in a prop.
/testenv up |
@kgabryje Ephemeral environment spinning up at http://35.86.252.223:8080. Credentials are |
Codecov Report
@@ Coverage Diff @@
## master #22207 +/- ##
==========================================
- Coverage 67.00% 66.99% -0.01%
==========================================
Files 1835 1835
Lines 69971 69976 +5
Branches 7590 7592 +2
==========================================
+ Hits 46881 46882 +1
- Misses 21124 21128 +4
Partials 1966 1966
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
LGTM
Ephemeral environment shutdown and build artifacts deleted. |
(cherry picked from commit 1809d2b)
🏷️ preset:2022.47 |
SUMMARY
This PR fixes 2 issues:
metric
control value from a string to an array. That resulted inUnhashable type: 'list' error
.undefined
error. That's because of the fact that we were expecting all the controls from the previous viz type to still be available and have values, which might not be the case after changing viz type.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
Screen.Recording.2022-11-23.at.17.28.30.mov
Screen.Recording.2022-11-23.at.17.29.10.mov
After:
Screen.Recording.2022-11-23.at.17.35.58.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION