fix: optimize mapStateToProps for chart controls #10264
Conversation
| if (controlPanelState.controls || !controlPanelState.isInitializing) { | ||
| applyMapStateToPropsToControl(controlState, controlPanelState); | ||
| } |
There was a problem hiding this comment.
you could make this something like:
const controlStateWithControlPanelProps = controlPanelState.controls || !controlPanelState.isInitializing
? applyMapStateToPropsToControl(controlState, controlPanelState)
: controlState;and then use it below. With that context, it should be fairly clear that we want to continue to use the latest created state variable. especially since the function isn't massive, I don't think people would mistakenly use the old state in this function
There was a problem hiding this comment.
I've changed it to use let. Not a big fan of having many variables. Always struggled with variable names and it quickly becomes confusing.
| const slice = bootstrappedState.slice; | ||
| const sliceName = slice ? slice.slice_name : null; | ||
| // apply initial mapStateToProps for all controls, must execute AFTER | ||
| // bootstrappedState has initialized `controls`. Order of execution is not |
There was a problem hiding this comment.
why order of execution is not guaranteed? it looks like a regular for loop?
There was a problem hiding this comment.
I thought Object.keys(...) does not guarantee property order? Even if it does it's probably not a good idea to rely on it anyway.
mistercrunch
added
🏷️ bot
SUMMARY
This is a bug fix for "EXPLORE" button in SQL query results.
The query mode control for table chart relies on
controlsprop in control panel state to determine what's the actual query mode, socontrolsstate mapping for chart controls must be initialized beforemapStateToPropsis called.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
When clicking on the "EXPLORE" button in SQL Lab:
Before:
Query is successful, but query mode is incorrectly set to "Aggregate":
After:
Query successfully switches to "Raw" mode:
TEST PLAN
Updated unit test + manual testing
ADDITIONAL INFORMATION
cc @kristw @serenajiang