fix(explore): only refresh data panel on relevant changes #16699
Conversation
| if (!props.triggerRender) { | ||
| setQueryFormData(props.chart.latestQueryFormData); | ||
| } | ||
| }, [props.chart.latestQueryFormData]); |
There was a problem hiding this comment.
Should we include triggerRender in deps array?
There was a problem hiding this comment.
No - that would cause it to trigger when we change controls whose renderTrigger prop is set to true.
There was a problem hiding this comment.
that would run useEffect function, but we'd exit at if check, right?
There was a problem hiding this comment.
Right you are - adding it!
There was a problem hiding this comment.
Actually, this causes a rerender when making changes to controls that don't have renderTrigger set to true (a rerendering should only trigger after running the query):
https://user-images.githubusercontent.com/33317356/133249851-49e9ceb3-0865-4172-b915-0c2c2b0c5bdc.mp4
There was a problem hiding this comment.
Got it, thanks for explanation! In that case, can we add eslint suppress warning? I think it's time we started getting rid of those errors wherever possible 🙂 (though actually that's up for debate if we should suppress those warnings...)
There was a problem hiding this comment.
I added a comment + suppressed the warning 👍
Codecov Report
@@ Coverage Diff @@
## master #16699 +/- ##
==========================================
- Coverage 76.93% 76.93% -0.01%
==========================================
Files 1007 1007
Lines 54112 54117 +5
Branches 7346 7348 +2
==========================================
+ Hits 41633 41634 +1
- Misses 12239 12243 +4
Partials 240 240
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
* fix(explore): only refresh data panel on relevant changes * add comment and supress warning (cherry picked from commit c99cacb)
* fix(explore): only refresh data panel on relevant changes * add comment and supress warning
* fix(explore): only refresh data panel on relevant changes * add comment and supress warning
mistercrunch
added
🍒 1.3.1
🍒 1.3.2
🏷️ bot
SUMMARY
Every time control state is updated on the Explore view, the data panel requests new data, even when the changes are known not to affect the chart data (
renderTriggerproperty set totrue). This PR changesExploreChartPanelto only refresh thequeryFormDatathat's used byDataTablesPaneif thetriggerRenderprop is nottrue(is falsy when a query has been issued). This only happens when the chart needs to be rerendered without rerequesting data.AFTER
before2.mp4
BEFORE
before.mp4
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION