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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
