graceguo-supercat commented on a change in pull request #4459: [Explore] 
highlighting run query when chart is stale on explore view
URL: 
https://github.com/apache/incubator-superset/pull/4459#discussion_r170382897
 
 

 ##########
 File path: 
superset/assets/javascripts/explore/components/ExploreViewContainer.jsx
 ##########
 @@ -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 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, check 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. Every 
time there is a props change, *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 change. 
   
   

----------------------------------------------------------------
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:
us...@infra.apache.org


With regards,
Apache Git Services

Reply via email to