feat: show more information when loading chart #27255
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #27255 +/- ##
==========================================
+ Coverage 67.22% 67.35% +0.12%
==========================================
Files 1905 1909 +4
Lines 74577 74557 -20
Branches 8338 8325 -13
==========================================
+ Hits 50138 50218 +80
+ Misses 22389 22288 -101
- Partials 2050 2051 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
betodealmeida
force-pushed
the
sc-75386
branch
from
7580c07
to
55f38de
Compare
| @@ -232,16 +246,51 @@ class Chart extends React.PureComponent { | |||
| ); | |||
| } | |||
|
|
|||
| renderSpinner(chartStatus, databaseName) { | |||
| const messages = { | |||
| loading: t('Fetching data from %s', databaseName), | |||
There was a problem hiding this comment.
Not sure if we got design input here, but maybe we want to make it more clear that we're waiting for the database to do its job. There's always been this perception of "superset is slow" when in fact it's the underlying datastore that we're waiting on. "Fetching" is an active word and makes it sounds like Superset is the [slow] actor. Maybe a more passive message like "Querying Presto" or "Waiting on Presto...". @kasiazjc
Also curious if this is still an overlay (I think previously was transparent?) or just a opaque thing? Maybe a nice effect/animation would be slick here (?) I'm generally not too big on animation, but since this is so common and visible, it may make sense to have something subtle and sharp-looking here.
There was a problem hiding this comment.
That's a great point, and makes all the difference!
There was a problem hiding this comment.
Agree with @mistercrunch here. A passive message will help improve Superset's performance perception.
|
/testenv up |
| errorMessage, | ||
| chartIsStale, | ||
| queriesResponse = [], | ||
| width, | ||
| } = this.props; | ||
| const databaseName = datasource?.database?.name; |
There was a problem hiding this comment.
According to Typescript the datasource/database can be undefined which will show in the interface. Can we handle this case?
There was a problem hiding this comment.
Maybe use "Waiting on database..." when the database name is not available.
|
@michael-s-molina Ephemeral environment spinning up at http://34.216.78.0:8080. Credentials are |
|
Not in the scope of this PR but related. Should we make the same change on SQL Lab for the queries? @justinpark for awareness. |
| )} | ||
| </div> | ||
| {isLoading && <Loading />} | ||
| {isLoading |
There was a problem hiding this comment.
const isLoading = chartStatus === 'loading';
This means that this.renderSpinner(chartStatus, databaseName) will never be called with another status which makes the other status messages irrelevant. I think having only 'Waiting on {database}' is sufficient.
There was a problem hiding this comment.
D'oh, you're right, let me fix this. Thanks!
|
/testenv up |
|
@yousoph Ephemeral environment spinning up at http://18.246.149.129:8080. Credentials are |
|
Ephemeral environment shutdown and build artifacts deleted. |
(cherry picked from commit fbc8943)
(cherry picked from commit fbc8943)
(cherry picked from commit fbc8943)
(cherry picked from commit fbc8943)
(cherry picked from commit fbc8943)
(cherry picked from commit fbc8943)
(cherry picked from commit fbc8943)
(cherry picked from commit fbc8943)
(cherry picked from commit fbc8943)
(cherry picked from commit fbc8943)
SUMMARY
This PR adds text to the chart spinner, informing the user:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Dashboards:
Charts:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION