chore: refactor Alert-related components

Chiseling at #31590 that has gotten big / unruly, in this PR is a refactor of Alert-related components, going vanilla AntD. Also. Deprecating colors.alerts since it's ambiguous/redundant with warning/error and does not exist in antd-v5
apache · Jan 21, 2025 · 272e0b0 · 272e0b0
1 parent eec3744
commit 272e0b0
Show file tree

Hide file tree

Showing 40 changed files with 571 additions and 726 deletions.
diff --git a/superset-frontend/packages/superset-ui-core/src/style/index.tsx b/superset-frontend/packages/superset-ui-core/src/style/index.tsx
@@ -98,13 +98,6 @@ const defaultTheme = {
       light2: '#FAEDEE',
     },
     warning: {
-      base: '#FF7F44',
-      dark1: '#BF5E33',
-      dark2: '#7F3F21',
-      light1: '#FEC0A1',
-      light2: '#FFF2EC',
-    },
-    alert: {
       base: '#FCC700',
       dark1: '#BC9501',
       dark2: '#7D6300',

diff --git a/superset-frontend/plugins/legacy-plugin-chart-calendar/src/ReactCalendar.jsx b/superset-frontend/plugins/legacy-plugin-chart-calendar/src/ReactCalendar.jsx
@@ -157,13 +157,13 @@ export default styled(Calendar)`
     }
 
     .cal-heatmap-container .q1 {
-      background-color: ${theme.colors.alert.light2};
-      fill: ${theme.colors.alert.light2};
+      background-color: ${theme.colors.warning.light2};
+      fill: ${theme.colors.warning.light2};
     }
 
     .cal-heatmap-container .q2 {
-      background-color: ${theme.colors.alert.light1};
-      fill: ${theme.colors.alert.light1};
+      background-color: ${theme.colors.warning.light1};
+      fill: ${theme.colors.warning.light1};
     }
 
     .cal-heatmap-container .q3 {

diff --git a/superset-frontend/src/components/Alert/Alert.test.tsx b/superset-frontend/src/components/Alert/Alert.test.tsx
@@ -27,19 +27,17 @@ test('renders with default props', async () => {
   render(<Alert message="Message" />);
 
   expect(screen.getByRole('alert')).toHaveTextContent('Message');
-  expect(await screen.findByLabelText('info icon')).toBeInTheDocument();
-  expect(await screen.findByLabelText('close icon')).toBeInTheDocument();
+  expect(screen.getByRole('img', { name: 'info-circle' })).toBeInTheDocument();
 });
 
-test('renders each type', async () => {
+test('renders message for each alert type', () => {
   const types: AlertTypeValue[] = ['info', 'error', 'warning', 'success'];
 
-  await Promise.all(
-    types.map(async type => {
-      render(<Alert type={type} message="Message" />);
-      expect(await screen.findByLabelText(`${type} icon`)).toBeInTheDocument();
-    }),
-  );
+  types.forEach(type => {
+    const { rerender } = render(<Alert type={type} message="Test message" />);
+    expect(screen.getByText('Test message')).toBeInTheDocument();
+    rerender(<></>); // Clean up between renders
+  });
 });
 
 test('renders without close button', async () => {
@@ -51,7 +49,7 @@ test('renders without close button', async () => {
 
 test('disappear when closed', async () => {
   render(<Alert message="Message" />);
-  userEvent.click(screen.getByLabelText('close icon'));
+  userEvent.click(screen.getByRole('img', { name: 'close' }));
   await waitFor(() => {
     expect(screen.queryByRole('alert')).not.toBeInTheDocument();
   });
@@ -74,6 +72,6 @@ test('renders message and description', async () => {
 test('calls onClose callback when closed', () => {
   const onCloseMock = jest.fn();
   render(<Alert message="Message" onClose={onCloseMock} />);
-  userEvent.click(screen.getByLabelText('close icon'));
+  userEvent.click(screen.getByRole('img', { name: 'close' }));
   expect(onCloseMock).toHaveBeenCalledTimes(1);
 });
diff --git a/superset-frontend/src/components/Alert/index.tsx b/superset-frontend/src/components/Alert/index.tsx
@@ -19,8 +19,6 @@
 import { PropsWithChildren } from 'react';
 import { Alert as AntdAlert } from 'antd-v5';
 import { AlertProps as AntdAlertProps } from 'antd-v5/lib/alert';
-import { css, useTheme } from '@superset-ui/core';
-import Icons from 'src/components/Icons';
 
 export type AlertProps = PropsWithChildren<
   Omit<AntdAlertProps, 'children'> & { roomBelow?: boolean }
@@ -32,59 +30,17 @@ export default function Alert(props: AlertProps) {
     description,
     showIcon = true,
     closable = true,
-    roomBelow = false,
     children,
   } = props;
 
-  const theme = useTheme();
-  const { colors } = theme;
-  const { alert: alertColor, error, info, success } = colors;
-
-  let baseColor = info;
-  let AlertIcon = Icons.InfoSolid;
-  if (type === 'error') {
-    baseColor = error;
-    AlertIcon = Icons.ErrorSolid;
-  } else if (type === 'warning') {
-    baseColor = alertColor;
-    AlertIcon = Icons.AlertSolid;
-  } else if (type === 'success') {
-    baseColor = success;
-    AlertIcon = Icons.CircleCheckSolid;
-  }
-
   return (
     <AntdAlert
       role="alert"
       aria-live={type === 'error' ? 'assertive' : 'polite'}
       showIcon={showIcon}
-      icon={
-        showIcon && (
-          <span
-            role="img"
-            aria-label={`${type} icon`}
-            style={{
-              color: baseColor.base,
-            }}
-          >
-            <AlertIcon />
-          </span>
-        )
-      }
-      closeIcon={closable && <Icons.XSmall aria-label="close icon" />}
+      closeIcon={closable}
       message={children || 'Default message'}
       description={description}
-      css={css`
-        margin-bottom: ${roomBelow ? theme.gridUnit * 4 : 0}px;
-        a {
-          text-decoration: underline;
-        }
-        .antd5-alert-message {
-          font-weight: ${description
-            ? theme.typography.weights.bold
-            : 'inherit'};
-        }
-      `}
       {...props}
     />
   );

diff --git a/superset-frontend/src/components/AlteredSliceTag/index.tsx b/superset-frontend/src/components/AlteredSliceTag/index.tsx
@@ -221,7 +221,7 @@ const AlteredSliceTag: FC<AlteredSliceTagProps> = props => {
         <Label
           icon={<Icons.Warning iconSize="m" />}
           className="label"
-          type="alert"
+          type="warning"
           onClick={() => {}}
         >
           {t('Altered')}

diff --git a/superset-frontend/src/components/Chart/Chart.tsx b/superset-frontend/src/components/Chart/Chart.tsx
@@ -24,6 +24,7 @@ import {
   logging,
   QueryFormData,
   styled,
+  ErrorTypeEnum,
   t,
   SqlaFormData,
   ClientErrorObject,
@@ -172,12 +173,6 @@ const MessageSpan = styled.span`
   color: ${({ theme }) => theme.colors.grayscale.base};
 `;
 
-const MonospaceDiv = styled.div`
-  font-family: ${({ theme }) => theme.typography.families.monospace};
-  word-break: break-word;
-  overflow-x: auto;
-  white-space: pre-wrap;
-`;
 class Chart extends PureComponent<ChartProps, {}> {
   static defaultProps = defaultProps;
 
@@ -245,7 +240,15 @@ class Chart extends PureComponent<ChartProps, {}> {
       height,
       datasetsStatus,
     } = this.props;
-    const error = queryResponse?.errors?.[0];
+    let error = queryResponse?.errors?.[0];
+    if (error === undefined) {
+      error = {
+        error_type: ErrorTypeEnum.FRONTEND_NETWORK_ERROR,
+        level: 'error',
+        message: t('Check your network connection'),
+        extra: null,
+      };
+    }
     const message = chartAlert || queryResponse?.message;
 
     // if datasource is still loading, don't render JS errors
@@ -273,8 +276,7 @@ class Chart extends PureComponent<ChartProps, {}> {
         key={chartId}
         chartId={chartId}
         error={error}
-        subtitle={<MonospaceDiv>{message}</MonospaceDiv>}
-        copyText={message}
+        subtitle={message}
         link={queryResponse ? queryResponse.link : undefined}
         source={dashboardId ? ChartSource.Dashboard : ChartSource.Explore}
         stackTrace={chartStackTrace}

diff --git a/superset-frontend/src/components/Chart/ChartErrorMessage.tsx b/superset-frontend/src/components/Chart/ChartErrorMessage.tsx
@@ -26,17 +26,14 @@ import { ChartSource } from 'src/types/ChartSource';
 export type Props = {
   chartId: string;
   error?: SupersetError;
-  subtitle: JSX.Element;
-  copyText: string | undefined;
+  subtitle: React.ReactNode;
   link?: string;
   source: ChartSource;
   stackTrace?: string;
 } & Omit<ClientErrorObject, 'error'>;
 
-/**
- * fetches the chart owners and adds them to the extra data of the error message
- */
 export const ChartErrorMessage: FC<Props> = ({ chartId, error, ...props }) => {
+  // fetches the chart owners and adds them to the extra data of the error message
   const { result: owners } = useChartOwnerNames(chartId);
 
   // don't mutate props

diff --git a/superset-frontend/src/components/ErrorBoundary/index.tsx b/superset-frontend/src/components/ErrorBoundary/index.tsx
@@ -18,7 +18,7 @@
  */
 import { Component, ErrorInfo, ReactNode } from 'react';
 import { t } from '@superset-ui/core';
-import ErrorMessageWithStackTrace from 'src/components/ErrorMessage/ErrorMessageWithStackTrace';
+import ErrorAlert from 'src/components/ErrorMessage/ErrorAlert';
 
 export interface ErrorBoundaryProps {
   children: ReactNode;
@@ -52,23 +52,13 @@ export default class ErrorBoundary extends Component<
   render() {
     const { error, info } = this.state;
     if (error) {
-      const firstLine = error.toString();
-      const messageString = `${t('Unexpected error')}${
-        firstLine ? `: ${firstLine}` : ''
-      }`;
-      const messageElement = (
-        <span>
-          <strong>{t('Unexpected error')}</strong>
-          {firstLine ? `: ${firstLine}` : ''}
-        </span>
-      );
-
+      const firstLine = error.toString().split('\n')[0];
       if (this.props.showMessage) {
         return (
-          <ErrorMessageWithStackTrace
-            subtitle={messageElement}
-            copyText={messageString}
-            stackTrace={info?.componentStack}
+          <ErrorAlert
+            errorType={t('Unexpected error')}
+            message={firstLine}
+            descriptionDetails={info?.componentStack}
           />
         );
       }

diff --git a/superset-frontend/src/components/ErrorMessage/DatabaseErrorMessage.tsx b/superset-frontend/src/components/ErrorMessage/DatabaseErrorMessage.tsx
@@ -34,12 +34,16 @@ interface DatabaseErrorExtra {
 
 function DatabaseErrorMessage({
   error,
-  source = 'dashboard',
+  source,
   subtitle,
 }: ErrorMessageComponentProps<DatabaseErrorExtra | null>) {
   const { extra, level, message } = error;
 
-  const isVisualization = ['dashboard', 'explore'].includes(source);
+  const isVisualization = ['dashboard', 'explore'].includes(source || '');
+  const [firstLine, ...remainingLines] = message.split('\n');
+  const alertMessage = firstLine;
+  const alertDescription =
+    remainingLines.length > 0 ? remainingLines.join('\n') : null;
 
   const body = extra && (
     <>
@@ -75,23 +79,13 @@ function DatabaseErrorMessage({
     </>
   );
 
-  const copyText = extra?.issue_codes
-    ? t('%(message)s\nThis may be triggered by: \n%(issues)s', {
-        message,
-        issues: extra.issue_codes
-          .map(issueCode => issueCode.message)
-          .join('\n'),
-      })
-    : message;
-
   return (
     <ErrorAlert
-      title={t('%s Error', extra?.engine_name || t('DB engine'))}
-      subtitle={subtitle}
-      level={level}
-      source={source}
-      copyText={copyText}
-      body={body}
+      errorType={t('%s Error', extra?.engine_name || t('DB engine'))}
+      message={alertMessage}
+      description={alertDescription}
+      type={level}
+      descriptionDetails={body}
     />
   );
 }

diff --git a/superset-frontend/src/components/ErrorMessage/DatasetNotFoundErrorMessage.tsx b/superset-frontend/src/components/ErrorMessage/DatasetNotFoundErrorMessage.tsx
@@ -23,19 +23,15 @@ import ErrorAlert from './ErrorAlert';
 
 function DatasetNotFoundErrorMessage({
   error,
-  source = 'dashboard',
   subtitle,
 }: ErrorMessageComponentProps) {
   const { level, message } = error;
-
   return (
     <ErrorAlert
-      title={t('Missing dataset')}
-      subtitle={subtitle}
-      level={level}
-      source={source}
-      copyText={message}
-      body={null}
+      errorType={t('Missing dataset')}
+      message={subtitle}
+      description={message}
+      type={level}
     />
   );
 }