SBIN2010 commented on code in PR #45003:
URL: https://github.com/apache/superset/pull/45003#discussion_r4200194845
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -492,16 +496,32 @@ export const ControlPanelsContainer = (props:
ControlPanelsContainerProps) => {
useEffect(() => {
if (
prevDatasource &&
- prevDatasource.type !== DatasourceType.Query &&
(props.exploreState.datasource?.id !== prevDatasource.id ||
props.exploreState.datasource?.type !== prevDatasource.type)
) {
- setShowDatasourceAlert(true);
- containerRef.current?.scrollTo(0, 0);
+ if (
+ prevDatasource.type !== DatasourceType.Query &&
+ !props.exploreState.skipDatasetChangeAlert
+ ) {
+ setDatasourceAlert({
+ controlsTransferred: [
+ ...ensureIsArray(props.exploreState.controlsTransferred),
+ ],
+ isCurrentDataset: true,
+ });
+ containerRef.current?.scrollTo(0, 0);
+ } else {
+ // Skipping this notification does not acknowledge an earlier one.
+ setDatasourceAlert(alert =>
+ alert ? { ...alert, isCurrentDataset: false } : null,
+ );
Review Comment:
The `else` branch runs for two different cases: `skipDatasetChangeAlert` and
a previous SQL Lab query datasource. Previously a `DatasourceType.Query`
transition left an already-open alert untouched. Now it rewrites that alert as
historical and removes **Clear form**, even when the caller did not pass
`skipDatasetChangeAlert`.
Please keep the query exemption a no-op and only demote an existing alert
for an explicit skip:
```tsx
if (prevDatasource.type === DatasourceType.Query) {
return;
}
if (props.exploreState.skipDatasetChangeAlert) {
setDatasourceAlert(alert =>
alert ? { ...alert, isCurrentDataset: false } : null,
);
return;
}
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -839,34 +860,55 @@ export const ControlPanelsContainer = (props:
ControlPanelsContainerProps) => {
};
const hasControlsTransferred =
- ensureIsArray(props.exploreState.controlsTransferred).length > 0;
+ (datasourceAlert?.controlsTransferred.length ?? 0) > 0;
const DatasourceAlert = useCallback(
() =>
hasControlsTransferred ? (
<ExploreAlert
title={t('Keep control settings?')}
Review Comment:
When `isCurrentDataset` is false, the body describes a previous change, but
the title stays in the present tense (`Keep control settings?` / `No form
settings were maintained`). **Clear form** is gone, so the title asks for a
choice the user can no longer make, and it reads as if it applies to the
dataset that is current now.
Branch the title the same way as `bodyText`, for example `Earlier dataset
change` / `Earlier dataset change did not keep settings`, and update the
store-backed test that still expects the original title after a skipped change.
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -492,16 +496,32 @@ export const ControlPanelsContainer = (props:
ControlPanelsContainerProps) => {
useEffect(() => {
if (
prevDatasource &&
- prevDatasource.type !== DatasourceType.Query &&
(props.exploreState.datasource?.id !== prevDatasource.id ||
props.exploreState.datasource?.type !== prevDatasource.type)
) {
- setShowDatasourceAlert(true);
- containerRef.current?.scrollTo(0, 0);
+ if (
+ prevDatasource.type !== DatasourceType.Query &&
+ !props.exploreState.skipDatasetChangeAlert
+ ) {
+ setDatasourceAlert({
+ controlsTransferred: [
+ ...ensureIsArray(props.exploreState.controlsTransferred),
+ ],
+ isCurrentDataset: true,
+ });
+ containerRef.current?.scrollTo(0, 0);
+ } else {
+ // Skipping this notification does not acknowledge an earlier one.
+ setDatasourceAlert(alert =>
+ alert ? { ...alert, isCurrentDataset: false } : null,
+ );
+ }
}
}, [
props.exploreState.datasource?.id,
props.exploreState.datasource?.type,
+ props.exploreState.skipDatasetChangeAlert,
+ props.exploreState.controlsTransferred,
Review Comment:
`controlsTransferred` does not need to be a dependency. The snapshot is
taken from the render where the datasource id or type changes, and that render
already closes over the new list. A later change to the list does not refresh
the snapshot, because the id check fails once `prevDatasource` has caught up.
Dropping it keeps the effect tied to the transition.
--
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]
