[Explore] highlighting run query when chart is stale on explore view #4459
Conversation
|
For future work I want to make it more obvious that the chart should be refreshed by overlaying the chart with a refresh button. |
gabe-lyons
force-pushed
the
gabe_highlighting_run_query_when_chart_is_stale
branch
from
dd167ff
to
032d8c1
Compare
|
I like this idea, this generally looks good but Grace is more familiar with this part of the code and may know more if there are any edge cases we aren't thinking of. |
| return Object.keys(currentControls).some(key => ( | ||
| currentControls[key].renderTrigger && | ||
| typeof prevControls[key] !== 'undefined' && | ||
| !areObjectsEqual(currentControls[key].value, prevControls[key].value) | ||
| )); | ||
| } | ||
|
|
||
| hasQueryControlChanged(prevControls, currentControls) { |
There was a problem hiding this 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.
gabe-lyons
force-pushed
the
gabe_highlighting_run_query_when_chart_is_stale
branch
from
6c00401
to
584994c
Compare
|
@GabeLoins in the demo you showed you had an overlay on the chart itself and I don't see it in the code here. Were you further in your branch? Unrelated to this PR: I was just thinking that this overaly (from memory it was centered, and the background opacity was ligther) could be used for the loading (spinner) animation as well. |
|
@graceguo-supercat addressed! Ready to merge. @mistercrunch yeah, thats coming in PR #2 |
…pache#4459) * highlighting run query when chart is stale on explore view * refactoring control changed code
…pache#4459) * highlighting run query when chart is stale on explore view * refactoring control changed code
mistercrunch
added
🏷️ bot
This PR does two things:
makes the Run Query grey by default- we don't want to highlight this button unless it would actually provide a new result.
highlights the Run Query button when a change to the query powering the chart is made. Currently I determine a change is affecting the query if
renderTriggeris falsey in the component's config.open questions:
Before query change button is blank:
After query change button pops:
reviewers:
@graceguo-supercat
@michellethomas
@mistercrunch