-
Notifications
You must be signed in to change notification settings - Fork 14.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[cache] Render label when cached #7164
[cache] Render label when cached #7164
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7164 +/- ##
==========================================
+ Coverage 64.63% 64.64% +<.01%
==========================================
Files 422 422
Lines 20607 20606 -1
Branches 2253 2253
==========================================
Hits 13320 13320
+ Misses 7165 7164 -1
Partials 122 122
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #7164 +/- ##
=======================================
Coverage 64.62% 64.62%
=======================================
Files 422 422
Lines 20593 20593
Branches 2253 2253
=======================================
Hits 13309 13309
Misses 7161 7161
Partials 123 123
Continue to review full report at Codecov.
|
@@ -94,7 +94,6 @@ class ExploreChartHeader extends React.PureComponent { | |||
chartUpdateStartTime, | |||
latestQueryFormData, | |||
queryResponse } = this.props.chart; | |||
const chartSucceeded = ['success', 'rendered'].indexOf(this.props.chart.chartStatus) > 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please ignore previous review. i read it wrong...
<RowCountLabel | ||
rowcount={queryResponse.rowcount} | ||
limit={formData.row_limit} | ||
/>} | ||
{chartSucceeded && queryResponse && queryResponse.is_cached && | ||
{queryResponse && queryResponse.is_cached && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic here says we should only render cache label when
- fetch data succeeded, and
- has query response, and
- queryResponse.is_cached is true.
in No data case, we should not render cache label, and should not allow or ask user to force refresh.
@graceguo-supercat |
@john-bodley Thanks for your correction, |
0d53dc0
to
e72d658
Compare
@graceguo-supercat I addressed your comments. |
e72d658
to
79e467d
Compare
A result set may return zero rows which is then cached (as expected) however this isn't evident in the explorer UI and thus there's no way to force refresh the query.
Note this also fixes an issue where the
success
state was ignored given we were checking anindexOf
being strictly greater than zero.Before
After
to: @kristw @michellethomas @mistercrunch