fix: Change dropdown in Alert/Report modal to use javascript for cond…

…itional rendering instead of css (#22360)
apache · Dec 8, 2022 · 49f1cfc · 49f1cfc
1 parent e1ef9e0
commit 49f1cfc
Show file tree

Hide file tree

Showing 3 changed files with 72 additions and 40 deletions.
diff --git a/superset-frontend/src/views/CRUD/alert/AlertReportModal.test.jsx b/superset-frontend/src/views/CRUD/alert/AlertReportModal.test.jsx
@@ -180,11 +180,11 @@ describe('AlertReportModal', () => {
     expect(wrapper.find('input[name="name"]')).toExist();
   });
 
-  it('renders five select elements when in report mode', () => {
+  it('renders four select elements when in report mode', () => {
     expect(wrapper.find(Select)).toExist();
     expect(wrapper.find(AsyncSelect)).toExist();
     expect(wrapper.find(Select)).toHaveLength(2);
-    expect(wrapper.find(AsyncSelect)).toHaveLength(3);
+    expect(wrapper.find(AsyncSelect)).toHaveLength(2);
   });
 
   it('renders Switch element', () => {
@@ -220,14 +220,14 @@ describe('AlertReportModal', () => {
     expect(input.props().initialValue).toEqual('SELECT NaN');
   });
 
-  it('renders five select element when in report mode', () => {
+  it('renders four select element when in report mode', () => {
     expect(wrapper.find(Select)).toExist();
     expect(wrapper.find(AsyncSelect)).toExist();
     expect(wrapper.find(Select)).toHaveLength(2);
-    expect(wrapper.find(AsyncSelect)).toHaveLength(3);
+    expect(wrapper.find(AsyncSelect)).toHaveLength(2);
   });
 
-  it('renders seven select elements when in alert mode', async () => {
+  it('renders six select elements when in alert mode', async () => {
     const props = {
       ...mockedProps,
       isReport: false,
@@ -238,7 +238,7 @@ describe('AlertReportModal', () => {
     expect(addWrapper.find(Select)).toExist();
     expect(addWrapper.find(AsyncSelect)).toExist();
     expect(addWrapper.find(Select)).toHaveLength(3);
-    expect(addWrapper.find(AsyncSelect)).toHaveLength(4);
+    expect(addWrapper.find(AsyncSelect)).toHaveLength(3);
   });
 
   it('renders value input element when in alert mode', async () => {

diff --git a/superset-frontend/src/views/CRUD/alert/AlertReportModal.test.tsx b/superset-frontend/src/views/CRUD/alert/AlertReportModal.test.tsx
@@ -40,3 +40,37 @@ test('allows change to None in log retention', async () => {
   // check if None is selected
   expect(selectedItem).toHaveTextContent('None');
 });
+
+test('renders the appropriate dropdown in Message Content section', async () => {
+  render(<AlertReportModal show />, { useRedux: true });
+
+  const chartRadio = screen.getByRole('radio', { name: /chart/i });
+
+  // Dashboard is initially checked by default
+  expect(
+    await screen.findByRole('radio', {
+      name: /dashboard/i,
+    }),
+  ).toBeChecked();
+  expect(chartRadio).not.toBeChecked();
+  // Only the dashboard dropdown should show
+  expect(screen.getByRole('combobox', { name: /dashboard/i })).toBeVisible();
+  expect(
+    screen.queryByRole('combobox', { name: /chart/i }),
+  ).not.toBeInTheDocument();
+
+  // Click the chart radio option
+  userEvent.click(chartRadio);
+
+  expect(await screen.findByRole('radio', { name: /chart/i })).toBeChecked();
+  expect(
+    screen.getByRole('radio', {
+      name: /dashboard/i,
+    }),
+  ).not.toBeChecked();
+  // Now that chart is checked, only the chart dropdown should show
+  expect(screen.getByRole('combobox', { name: /chart/i })).toBeVisible();
+  expect(
+    screen.queryByRole('combobox', { name: /dashboard/i }),
+  ).not.toBeInTheDocument();
+});
diff --git a/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx b/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx
@@ -1314,40 +1314,38 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
               <StyledRadio value="dashboard">{t('Dashboard')}</StyledRadio>
               <StyledRadio value="chart">{t('Chart')}</StyledRadio>
             </Radio.Group>
-            <AsyncSelect
-              ariaLabel={t('Chart')}
-              css={{
-                display: contentType === 'chart' ? 'inline' : 'none',
-              }}
-              name="chart"
-              value={
-                currentAlert?.chart?.label && currentAlert?.chart?.value
-                  ? {
-                      value: currentAlert.chart.value,
-                      label: currentAlert.chart.label,
-                    }
-                  : undefined
-              }
-              options={loadChartOptions}
-              onChange={onChartChange}
-            />
-            <AsyncSelect
-              ariaLabel={t('Dashboard')}
-              css={{
-                display: contentType === 'dashboard' ? 'inline' : 'none',
-              }}
-              name="dashboard"
-              value={
-                currentAlert?.dashboard?.label && currentAlert?.dashboard?.value
-                  ? {
-                      value: currentAlert.dashboard.value,
-                      label: currentAlert.dashboard.label,
-                    }
-                  : undefined
-              }
-              options={loadDashboardOptions}
-              onChange={onDashboardChange}
-            />
+            {contentType === 'chart' ? (
+              <AsyncSelect
+                ariaLabel={t('Chart')}
+                name="chart"
+                value={
+                  currentAlert?.chart?.label && currentAlert?.chart?.value
+                    ? {
+                        value: currentAlert.chart.value,
+                        label: currentAlert.chart.label,
+                      }
+                    : undefined
+                }
+                options={loadChartOptions}
+                onChange={onChartChange}
+              />
+            ) : (
+              <AsyncSelect
+                ariaLabel={t('Dashboard')}
+                name="dashboard"
+                value={
+                  currentAlert?.dashboard?.label &&
+                  currentAlert?.dashboard?.value
+                    ? {
+                        value: currentAlert.dashboard.value,
+                        label: currentAlert.dashboard.label,
+                      }
+                    : undefined
+                }
+                options={loadDashboardOptions}
+                onChange={onDashboardChange}
+              />
+            )}
             {formatOptionEnabled && (
               <>
                 <div className="inline-container">