fix(list/chart views): Chart Properties modal now has transitions (#2…

…8796) Co-authored-by: JUST.in DO IT <justin.park@airbnb.com>
apache · Jul 31, 2024 · 66eb959 · 66eb959
1 parent 59e366c
commit 66eb959
Show file tree

Hide file tree

Showing 4 changed files with 51 additions and 48 deletions.
diff --git a/superset-frontend/src/explore/components/PropertiesModal/index.tsx b/superset-frontend/src/explore/components/PropertiesModal/index.tsx
@@ -69,7 +69,7 @@ function PropertiesModal({
   const [submitting, setSubmitting] = useState(false);
   const [form] = AntdForm.useForm();
   // values of form inputs
-  const [name, setName] = useState(slice.slice_name || '');
+  const [name, setName] = useState(slice?.slice_name || '');
   const [selectedOwners, setSelectedOwners] = useState<SelectValue | null>(
     null,
   );
@@ -98,23 +98,25 @@ function PropertiesModal({
 
   const fetchChartOwners = useCallback(
     async function fetchChartOwners() {
-      try {
-        const response = await SupersetClient.get({
-          endpoint: `/api/v1/chart/${slice.slice_id}`,
-        });
-        const chart = response.json.result;
-        setSelectedOwners(
-          chart?.owners?.map((owner: any) => ({
-            value: owner.id,
-            label: `${owner.first_name} ${owner.last_name}`,
-          })),
-        );
-      } catch (response) {
-        const clientError = await getClientErrorObject(response);
-        showError(clientError);
+      if (slice?.slice_id) {
+        try {
+          const response = await SupersetClient.get({
+            endpoint: `/api/v1/chart/${slice?.slice_id}`,
+          });
+          const chart = response.json.result;
+          setSelectedOwners(
+            chart?.owners?.map((owner: any) => ({
+              value: owner.id,
+              label: `${owner.first_name} ${owner.last_name}`,
+            })),
+          );
+        } catch (response) {
+          const clientError = await getClientErrorObject(response);
+          showError(clientError);
+        }
       }
     },
-    [slice.slice_id],
+    [slice?.slice_id],
   );
 
   const loadOptions = useMemo(
@@ -175,7 +177,7 @@ function PropertiesModal({
 
     try {
       const res = await SupersetClient.put({
-        endpoint: `/api/v1/chart/${slice.slice_id}`,
+        endpoint: `/api/v1/chart/${slice?.slice_id}`,
         headers: { 'Content-Type': 'application/json' },
         body: JSON.stringify(payload),
       });
@@ -184,7 +186,7 @@ function PropertiesModal({
         ...payload,
         ...res.json.result,
         tags,
-        id: slice.slice_id,
+        id: slice?.slice_id,
         owners: selectedOwners,
       };
       onSave(updatedChart);
@@ -206,16 +208,16 @@ function PropertiesModal({
 
   // update name after it's changed in another modal
   useEffect(() => {
-    setName(slice.slice_name || '');
-  }, [slice.slice_name]);
+    setName(slice?.slice_name || '');
+  }, [slice?.slice_name]);
 
   useEffect(() => {
     if (!isFeatureEnabled(FeatureFlag.TaggingSystem)) return;
     try {
       fetchTags(
         {
           objectType: OBJECT_TYPES.CHART,
-          objectId: slice.slice_id,
+          objectId: slice?.slice_id,
           includeTypes: false,
         },
         (tags: TagType[]) => setTags(tags),
@@ -226,7 +228,7 @@ function PropertiesModal({
     } catch (error) {
       showError(error);
     }
-  }, [slice.slice_id]);
+  }, [slice?.slice_id]);
 
   const handleChangeTags = (tags: { label: string; value: number }[]) => {
     const parsedTags: TagType[] = ensureIsArray(tags).map(r => ({
@@ -262,9 +264,9 @@ function PropertiesModal({
             buttonSize="small"
             buttonStyle="primary"
             onClick={form.submit}
-            disabled={submitting || !name || slice.is_managed_externally}
+            disabled={submitting || !name || slice?.is_managed_externally}
             tooltip={
-              slice.is_managed_externally
+              slice?.is_managed_externally
                 ? t(
                     "This chart is managed externally, and can't be edited in Superset",
                   )
@@ -284,12 +286,13 @@ function PropertiesModal({
         onFinish={onSubmit}
         layout="vertical"
         initialValues={{
-          name: slice.slice_name || '',
-          description: slice.description || '',
-          cache_timeout: slice.cache_timeout != null ? slice.cache_timeout : '',
-          certified_by: slice.certified_by || '',
+          name: slice?.slice_name || '',
+          description: slice?.description || '',
+          cache_timeout:
+            slice?.cache_timeout != null ? slice.cache_timeout : '',
+          certified_by: slice?.certified_by || '',
           certification_details:
-            slice.certified_by && slice.certification_details
+            slice?.certified_by && slice?.certification_details
               ? slice.certification_details
               : '',
         }}

diff --git a/superset-frontend/src/features/home/ChartTable.tsx b/superset-frontend/src/features/home/ChartTable.tsx
@@ -171,15 +171,12 @@ function ChartTable({
   if (loading) return <LoadingCards cover={showThumbnails} />;
   return (
     <ErrorBoundary>
-      {sliceCurrentlyEditing && (
-        <PropertiesModal
-          onHide={closeChartEditModal}
-          onSave={handleChartUpdated}
-          show
-          slice={sliceCurrentlyEditing}
-        />
-      )}
-
+      <PropertiesModal
+        onHide={closeChartEditModal}
+        onSave={handleChartUpdated}
+        show={sliceCurrentlyEditing}
+        slice={sliceCurrentlyEditing}
+      />
       <SubMenu
         activeChild={activeTab}
         tabs={menuTabs}

diff --git a/superset-frontend/src/pages/ChartList/ChartList.test.jsx b/superset-frontend/src/pages/ChartList/ChartList.test.jsx
@@ -152,6 +152,11 @@ describe('ChartList', () => {
     expect(wrapper.find(ChartList)).toExist();
   });
 
+  it('renders, but PropertiesModal initially hidden', () => {
+    expect(wrapper.find(PropertiesModal).exists()).toBe(true);
+    expect(wrapper.find(PropertiesModal).prop('show')).toBe(false);
+  });
+
   it('renders a ListView', () => {
     expect(wrapper.find(ListView)).toExist();
   });
@@ -181,10 +186,10 @@ describe('ChartList', () => {
   });
 
   it('edits', async () => {
-    expect(wrapper.find(PropertiesModal)).not.toExist();
+    expect(wrapper.find(PropertiesModal).prop('show')).toBe(false);
     wrapper.find('[data-test="edit-alt"]').first().simulate('click');
     await waitForComponentToPaint(wrapper);
-    expect(wrapper.find(PropertiesModal)).toExist();
+    expect(wrapper.find(PropertiesModal).prop('show')).toBe(true);
   });
 
   it('delete', async () => {

diff --git a/superset-frontend/src/pages/ChartList/index.tsx b/superset-frontend/src/pages/ChartList/index.tsx
@@ -783,14 +783,12 @@ function ChartList(props: ChartListProps) {
   return (
     <>
       <SubMenu name={t('Charts')} buttons={subMenuButtons} />
-      {sliceCurrentlyEditing && (
-        <PropertiesModal
-          onHide={closeChartEditModal}
-          onSave={handleChartUpdated}
-          show
-          slice={sliceCurrentlyEditing}
-        />
-      )}
+      <PropertiesModal
+        onHide={closeChartEditModal}
+        onSave={handleChartUpdated}
+        show={!!sliceCurrentlyEditing}
+        slice={sliceCurrentlyEditing}
+      />
       <ConfirmStatusChange
         title={t('Please confirm')}
         description={t('Are you sure you want to delete the selected charts?')}