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 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
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
