graceguo-supercat commented on a change in pull request #4459: [Explore] 
highlighting run query when chart is stale on explore view

 File path: 
 @@ -121,14 +126,22 @@ class ExploreViewContainer extends React.Component {
     return `${window.innerHeight - navHeight}px`;
-  instantControlChanged(prevControls, currentControls) {
+  hasDisplayControlChanged(prevControls, currentControls) {
     return Object.keys(currentControls).some(key => (
       currentControls[key].renderTrigger &&
       typeof prevControls[key] !== 'undefined' &&
       !areObjectsEqual(currentControls[key].value, prevControls[key].value)
+  hasQueryControlChanged(prevControls, currentControls) {
 Review comment:
   `hasDisplayControlChange` and `hasQueryControlChange` has very similar 
logic, can you consolidate this 2 functions? Notice both 2 functions called 
`areObjectsEqual`, which is a function that stringify 2 JS object and then find 
diff. Similar to JSON.parse, this function call is not cheap, so we need to use 
it with caution.
   Also in componentWillReceiveProps we have a few logic block, it checks if 
viz_type changed, if data source changed, if control props changed, etc. The 
call to action creator, this.props.actions.ABCD, is not exit or return. This 
means every time a props changed, **all** of these logic will be checked. So if 
we do not consolidate 2 functions, the expensive compare function will be 
called twice every time there is a props change, no matter what kind of prop 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:

With regards,
Apache Git Services

Reply via email to