[GitHub] [superset] codyml commented on a diff in pull request #22835: feat: Enable new dataset creation flow II

["codyml (via GitHub)" <gi...@apache.org>]

codyml commented on code in PR #22835:
URL: https://github.com/apache/superset/pull/22835#discussion_r1092357804


##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -640,6 +642,14 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
     onHide();
   };
 
+  const redirectURL = (url: string) => {

Review Comment:
   Would you mind adding a `TODO` comment noting that this check and passing `history` as a prop can be removed once SQL Lab is in the SPA, as `history` will then be globally available?  Assuming that describes the situation correctly.



##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Footer/index.tsx:
##########
@@ -104,13 +100,13 @@ function Footer({
         if (typeof response === 'number') {
           logEvent(LOG_ACTIONS_DATASET_CREATION_SUCCESS, datasetObject);
           // When a dataset is created the response we get is its ID number
-          goToPreviousUrl();
+          history.push(`/chart/add/?dataset=${datasetObject.table_name}`);
         }
       });
     }
   };
 
-  const CREATE_DATASET_TEXT = t('Create Dataset');
+  const CREATE_DATASET_TEXT = t('Create Dataset and Create Chart');

Review Comment:
   Even though this appears in all caps, can we make it sentence case in the code for consistency and in case that CSS style ever gets changed?
   
   ```suggestion
     const CREATE_DATASET_TEXT = t('Create dataset and create chart');
   ```



##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Footer/index.tsx:
##########
@@ -104,13 +100,13 @@ function Footer({
         if (typeof response === 'number') {
           logEvent(LOG_ACTIONS_DATASET_CREATION_SUCCESS, datasetObject);
           // When a dataset is created the response we get is its ID number
-          goToPreviousUrl();
+          history.push(`/chart/add/?dataset=${datasetObject.table_name}`);
         }
       });
     }
   };
 
-  const CREATE_DATASET_TEXT = t('Create Dataset');
+  const CREATE_DATASET_TEXT = t('Create Dataset and Create Chart');

Review Comment:
   Also, I noticed that the tooltip nub is covered by the footer element when the button is disabled.  A quick fix if it doesn't break anything else would be to add `z-index: 0;` to the `FooterRow` styles in `styles.ts`.



-- 
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