EnxDev commented on code in PR #45541:
URL: https://github.com/apache/superset/pull/45541#discussion_r4341582175
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -543,7 +549,10 @@ function Chart({
height={height}
width={width}
>
- {showSpinner ? renderSpinner(databaseName) : renderChartContainer()}
+ {showSpinner && !keepChartOnScreen
+ ? renderSpinner(databaseName)
+ : renderChartContainer()}
+ {showSpinner && keepChartOnScreen && <Loading size="s" muted />}
Review Comment:
`Loading` defaults to `position="floating"`, which is `position: absolute`
at 50%/50% with a translate and `z-index: 99`, and the `Styles` wrapper it
renders into is `position: relative`. So it's already out of the flow and
centered over the chart. Passing the prop would only restate the default.
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -177,6 +177,28 @@ interface ChartHooks extends AsyncChartHooks {
onChartStateChange?: (chartState: AgGridChartState) => void;
}
+/**
+ * Whether a chart has a previous response it can stay on screen with while a
+ * new query is in flight.
+ */
+export function hasValidPreviousData(
+ queriesResponse: QueryData[] | null | undefined,
+ vizType: string,
+): boolean {
+ // A failed load leaves its errored response in the store, and the next
+ // CHART_UPDATE_STARTED clears chartAlert but not queriesResponse. Checking
+ // the response itself keeps that stale error from being drawn as chart data
+ // while the retry is in flight.
+ const hasAnyErrors = queriesResponse?.some(item => item?.error);
+ // The response of a chart that fetches its own data stays empty once it has
+ // loaded, so for those any response that landed counts as previous data.
+ const hasPreviousResponse =
+ (getChartMetadataRegistry().get(vizType)?.enableNoResults ?? true)
+ ? (queriesResponse?.length ?? 0) > 0
+ : Array.isArray(queriesResponse);
Review Comment:
Before the first load the response is `null`, not `[]`. The reducer's
initial chart state has `queriesResponse: null` and only
`CHART_UPDATE_SUCCEEDED` and `CHART_UPDATE_FAILED` write it, so `Array.isArray`
only turns true once a query has landed, which is the "loaded once" signal
you're describing. `does not render a self-fetching chart whose first query is
still in flight` covers it. `Chart` also only keeps a chart on screen when it
knows the form data the response was queried with, and a chart that never
loaded has none.
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -543,7 +570,10 @@ function Chart({
height={height}
width={width}
>
- {showSpinner ? renderSpinner(databaseName) : renderChartContainer()}
+ {showSpinner && !keepChartOnScreen
Review Comment:
Excluded them. `hasValidPreviousData` returns false when Matrixify is
enabled, so the chart goes back to the regular spinner and the cells remount
and fetch once the new response lands. That also covers auto-refresh, which had
the same gap since #45234. A refresh signal the cells could pick up would be
nicer, but `MatrixifyGridCell` and `StatefulChart` know nothing about `force`,
so it's a bigger change than this PR should carry. Test is `does not keep a
Matrixify chart on screen while it refreshes` in `ChartRenderer.test.tsx`,
fixed in 4babf24.
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -282,25 +307,23 @@ function ChartRendererComponent({
const chartEnableNoResults =
getChartMetadataRegistry().get(vizType)?.enableNoResults ?? true;
- // A failed load leaves its errored response in the store, and the next
- // CHART_UPDATE_STARTED clears chartAlert but not queriesResponse. Checking
- // the response itself keeps that stale error from being drawn as chart data
- // while the retry is in flight.
- const hasAnyErrors = queriesResponse?.some(item => item?.error);
- // The response of a chart that fetches its own data stays empty once it has
- // loaded, so for those any response that landed counts as previous data.
- const hasPreviousResponse = chartEnableNoResults
- ? (queriesResponse?.length ?? 0) > 0
- : Array.isArray(queriesResponse);
- const hasValidPreviousData = hasPreviousResponse && !hasAnyErrors;
-
- // An auto-refresh keeps the chart on screen while its query is in flight.
- // The store holds the previous response until the new one lands, so that
- // response is what the chart renders in the meantime.
+ // A dashboard keeps the chart on screen while its query is in flight, be it
+ // an auto-refresh, a manual refresh or a filter change. The store holds the
+ // previous response until the new one lands, so that response is what the
+ // chart renders in the meantime.
const isRefreshing =
- Boolean(restProps.suppressLoadingSpinner) && chartStatus === 'loading';
+ chartStatus === 'loading' &&
+ (Boolean(restProps.suppressLoadingSpinner) ||
+ source === ChartSource.Dashboard);
const hasRenderableResults =
- resultsReady || (isRefreshing && hasValidPreviousData);
+ resultsReady ||
+ (isRefreshing && hasValidPreviousData(queriesResponse, vizType));
Review Comment:
Yes. `Chart` drops the response's form data once the status goes to
`failed`, and `ChartRenderer` now only draws a previous response when that form
data is known, which also lines its gate up with the one in `Chart`. After a
render failure a manual refresh shows the regular spinner, and an auto-refresh
renders nothing until the new response lands. Both are covered by `does not
draw a response that failed to render again during ...` in
`ChartRenderer.refresh.test.tsx`. Fixed in 4babf24.
##########
superset-frontend/src/components/Chart/ChartRenderer.refresh.test.tsx:
##########
@@ -164,7 +177,177 @@ test('shows the new data once the auto-refresh query
returns', async () => {
/>,
);
- expect(await screen.findByText('value=2')).toBeInTheDocument();
+ expect(await screen.findByText(/value=2/)).toBeInTheDocument();
+});
+
+test('keeps a dashboard chart on screen while a manual refresh query is in
flight', async () => {
Review Comment:
Added `keeps a self-fetching dashboard chart mounted under the spinner
through a manual refresh`. It goes through `Chart` with `enableNoResults:
false` and `queriesResponse=[]`, steps `rendered` -> `loading` -> `success`
without spinner suppression, and asserts the plugin stays in the DOM next to
the loading indicator (no "Waiting on database..." placeholder) and mounts
exactly once. In 4babf24.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
