[GitHub] [superset] villebro commented on a diff in pull request #19696: feat(explore): Replace overlay with alert banner when chart controls change

[GitBox <gi...@apache.org>]

villebro commented on code in PR #19696:
URL: https://github.com/apache/superset/pull/19696#discussion_r852643855


##########
superset-frontend/src/components/Chart/Chart.jsx:
##########
@@ -255,34 +252,24 @@ class Chart extends React.PureComponent {
       chartAlert,
       chartStatus,
       errorMessage,
-      onQuery,
-      refreshOverlayVisible,
+      chartIsStale,
       queriesResponse = [],
       isDeactivatedViz = false,
       width,
     } = this.props;
 
     const isLoading = chartStatus === 'loading';
-    const isFaded = refreshOverlayVisible && !errorMessage;
+    const isFaded = chartIsStale && !errorMessage;

Review Comment:
   As we're no longer fading, this variable should probably be renamed



##########
superset-frontend/src/components/Chart/Chart.jsx:
##########
@@ -108,25 +107,23 @@ const Styles = styled.div`
   }
 `;
 
-const RefreshOverlayWrapper = styled.div`
-  position: absolute;
-  top: 0;
-  left: 0;
-  width: 100%;
-  height: 100%;
-  display: flex;
-  align-items: center;
-  justify-content: center;
-`;
-
 const MonospaceDiv = styled.div`
   font-family: ${({ theme }) => theme.typography.families.monospace};
-  white-space: pre;
   word-break: break-word;
   overflow-x: auto;
   white-space: pre-wrap;
 `;
 
+const requiredFieldsMissingWarning = isFeatureEnabled(
+  FeatureFlag.ENABLE_EXPLORE_DRAG_AND_DROP,
+)
+  ? t(
+      'Drag and drop values into highlighted field(s) in the control panel. Then run the query by clicking on the "Create chart" button.',
+    )
+  : t(
+      'Select values in highlighted field(s) in the control panel. Then run the query by clicking on the "Create chart" button.',
+    );

Review Comment:
   Not something you're changing here, but to get rid of a ternary, I wonder if we couldn't get away with just one message here. As the current default supports both dragging and dropping *and* clicking on the ghost button, the "Select values in highlighted field(s) in the control panel" message could IMO be an ok message for both DnD and non-DnD scenarios.



##########
superset-frontend/src/explore/components/ExploreChartPanel.jsx:
##########
@@ -109,6 +126,16 @@ const Styles = styled.div`
   }
 `;
 
+const requiredFieldsMissingWarning = isFeatureEnabled(
+  FeatureFlag.ENABLE_EXPLORE_DRAG_AND_DROP,
+)
+  ? t(
+      'Drag and drop values into highlighted field(s) in the control panel. Then run the query by clicking on the "Update chart" button.',
+    )
+  : t(
+      'Select values in highlighted field(s) in the control panel. Then run the query by clicking on the "Update chart" button.',
+    );

Review Comment:
   As this is identical to the text in `src/components/Chart/Chart.tsx`, we should perhaps consider placing these strings in a `constants.ts` to ensure they stay the same (otherwise someone might only update one but not the other).



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org