[GitHub] [superset] eric-briscoe commented on a diff in pull request #21557: fix: Dataset creation header is now uneditable and holds proper default values

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

eric-briscoe commented on code in PR #21557:
URL: https://github.com/apache/superset/pull/21557#discussion_r983904475


##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -71,7 +71,11 @@ export default function AddDataset() {
   >(datasetReducer, null);
 
   const HeaderComponent = () => (
-    <Header setDataset={setDataset} datasetName={dataset?.dataset_name ?? ''} />
+    <Header
+      setDataset={setDataset}
+      title={dataset?.table_name ?? 'New dataset'}

Review Comment:
   May want to wrap in a t('New dataset')
   Also, may be worth using exported const from previous comment if you want to keep this in sync with what the default title is for header.tsx.
   
   If making the title prop optional like mentioned in comment on Header/index.tsx then this could be simplified to `title={dataset?.table_name}` and the default value 'New dataset' would get applied by Header and schema prop could be removed
   



##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Header/index.tsx:
##########
@@ -59,21 +59,23 @@ const renderOverlay = () => (
 
 export default function Header({
   setDataset,
-  datasetName,
+  title,
+  schema,
 }: {
   setDataset: React.Dispatch<DSReducerActionType>;
-  datasetName: string;
+  title: string;
+  schema?: string | null | undefined;
 }) {
   const editableTitleProps = {
-    title: datasetName,
-    placeholder: t('Add the name of the dataset'),
+    title: schema ? title : t('New dataset'),
+    placeholder: t('New dataset'),

Review Comment:
   Since you are using t'(New dataset') twice here, it may be worth defining a const with that value and using it in both places.  If you export that const you could also use it in the test file so that if someone changes the value later you don't have to keep three separate string literals in sync manually.
   
   export const DEFAULT_TITLE = t(New dataset);
   
   Then use that on line 70, 71, and in line 51 of Header.test.tsx



##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Header/index.tsx:
##########
@@ -59,21 +59,23 @@ const renderOverlay = () => (
 
 export default function Header({
   setDataset,
-  datasetName,
+  title,
+  schema,
 }: {
   setDataset: React.Dispatch<DSReducerActionType>;
-  datasetName: string;
+  title: string;
+  schema?: string | null | undefined;
 }) {
   const editableTitleProps = {
-    title: datasetName,
-    placeholder: t('Add the name of the dataset'),
+    title: schema ? title : t('New dataset'),

Review Comment:
   curious why the usage of title here is tied to if schema is defined?  If you make title optional, then set a default value for title as t('New dataset') could the conditional logic here could be removed?
   
   `
   export const DEFAULT_TITLE = t(New dataset);
   export default function Header({
     setDataset,
     title = DEFAULT_TITLE,
   }: {
     setDataset: React.Dispatch<DSReducerActionType>;
     title?: string;
   }) {
     const editableTitleProps = {
       title,
       placeholder: DEFAULT_TITLE
   `
   
   It looks like the only use of schema prop is this case so changing this logic might also make the prop unnecessary and simplify the component's interface



##########
superset-frontend/src/views/CRUD/data/dataset/DatasetLayout/DatasetLayout.test.tsx:
##########
@@ -36,18 +36,12 @@ describe('DatasetLayout', () => {
   const mockSetDataset = jest.fn();
 
   const waitForRender = () =>
-    waitFor(() =>
-      render(<Header setDataset={mockSetDataset} datasetName="" />),
-    );
+    waitFor(() => render(<Header setDataset={mockSetDataset} title="" />));

Review Comment:
   I think the other comments I left might address this and allow the title="" to be removed



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