Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Dataset creation header is now uneditable and holds proper default values #21557

Merged
merged 8 commits into from
Oct 13, 2022
Merged

fix: Dataset creation header is now uneditable and holds proper default values #21557

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
df99293
rough draft of footer component
AAfghahi Aug 29, 2022
dfc8ec9
with create functionality
AAfghahi Sep 8, 2022
c0e47d8
header is now uneditable and holds proper default values
lyndsiWilliams Sep 20, 2022
ac9d80a
Fix DatasetLayout test
lyndsiWilliams Sep 22, 2022
f27495b
Fix AddDataset test
lyndsiWilliams Sep 22, 2022
d7b2969
Fix schema typing
lyndsiWilliams Sep 23, 2022
96235fe
Fix weird rebase issue
lyndsiWilliams Sep 28, 2022
e9f6eb2
Address review comments
lyndsiWilliams Oct 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions superset-frontend/src/views/CRUD/data/dataset/AddDataset/AddDataset.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,7 @@ describe('AddDataset', () => {
const blankeStateImgs = screen.getAllByRole('img', { name: /empty/i });

// Header
expect(
await screen.findByRole('textbox', {
name: /dataset name/i,
}),
).toBeVisible();
expect(await screen.findByTestId('editable-title')).toBeVisible();
// Left panel
expect(blankeStateImgs[0]).toBeVisible();
// Footer
Expand Down
47 changes: 15 additions & 32 deletions superset-frontend/src/views/CRUD/data/dataset/AddDataset/Header/Header.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,64 +16,47 @@
* specific language governing permissions and limitations
* under the License.
*/
import userEvent from '@testing-library/user-event';
import React from 'react';
import { render, screen, waitFor } from 'spec/helpers/testing-library';
import Header from 'src/views/CRUD/data/dataset/AddDataset/Header';

describe('Header', () => {
const mockSetDataset = jest.fn();

const waitForRender = (datasetName: string) =>
waitFor(() =>
render(<Header setDataset={mockSetDataset} datasetName={datasetName} />),
);
const waitForRender = (props?: any) =>
waitFor(() => render(<Header setDataset={mockSetDataset} {...props} />));

it('renders a blank state Header', async () => {
await waitForRender('');
test('renders a blank state Header', async () => {
await waitForRender();

const datasetNameTextbox = screen.getByRole('textbox', {
name: /dataset name/i,
});
const datasetName = screen.getByTestId('editable-title');
const saveButton = screen.getByRole('button', {
name: /save save/i,
});
const menuButton = screen.getByRole('button', {
name: /menu actions trigger/i,
});

expect(datasetNameTextbox).toBeVisible();
expect(datasetName).toBeVisible();
expect(saveButton).toBeVisible();
expect(saveButton).toBeDisabled();
expect(menuButton).toBeVisible();
expect(menuButton).toBeDisabled();
});

it('updates display value of dataset name textbox when Header title is changed', async () => {
await waitForRender('');

const datasetNameTextbox = screen.getByRole('textbox', {
name: /dataset name/i,
});
test('displays "New dataset" when a table is not selected', async () => {
await waitForRender();

// Textbox should start with an empty display value and placeholder text
expect(datasetNameTextbox).toHaveDisplayValue('');
expect(
screen.getByPlaceholderText(/add the name of the dataset/i),
).toBeVisible();

// Textbox should update its display value when user inputs a new value
userEvent.type(datasetNameTextbox, 'Test name');
expect(datasetNameTextbox).toHaveDisplayValue('Test name');
const datasetName = screen.getByTestId('editable-title');
expect(datasetName.innerHTML).toBe('New dataset');
});

it('passes an existing dataset title into the dataset name textbox', async () => {
await waitForRender('Existing Dataset Name');
test('displays table name when a table is selected', async () => {
// The schema and table name are passed in through props once selected
await waitForRender({ schema: 'testSchema', title: 'testTable' });

const datasetNameTextbox = screen.getByRole('textbox', {
name: /dataset name/i,
});
const datasetName = screen.getByTestId('editable-title');

expect(datasetNameTextbox).toHaveDisplayValue('Existing Dataset Name');
expect(datasetName.innerHTML).toBe('testTable');
});
});
12 changes: 7 additions & 5 deletions superset-frontend/src/views/CRUD/data/dataset/AddDataset/Header/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in this commit

placeholder: t('New dataset'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in this commit

onSave: (newDatasetName: string) => {
setDataset({
type: DatasetActionType.changeDataset,
payload: { name: 'dataset_name', value: newDatasetName },
});
},
canEdit: true,
canEdit: false,
label: t('dataset name'),
};

Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/views/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import { DatasetActionType } from '../types';

interface LeftPanelProps {
setDataset: Dispatch<SetStateAction<object>>;
schema?: string | undefined | null;
schema?: string | null | undefined;
dbId?: number;
}

Expand Down
6 changes: 5 additions & 1 deletion superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'}
eschutho marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in this commit

schema={dataset?.schema}
/>
);

const LeftPanelComponent = () => (
Expand Down
10 changes: 2 additions & 8 deletions superset-frontend/src/views/CRUD/data/dataset/DatasetLayout/DatasetLayout.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,12 @@ describe('DatasetLayout', () => {
const mockSetDataset = jest.fn();

const waitForRender = () =>
waitFor(() =>
render(<Header setDataset={mockSetDataset} datasetName="" />),
);
waitFor(() => render(<Header setDataset={mockSetDataset} title="" />));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this the same as having no title thus the title: string; should be optional? Or what implication would that have? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The title can't have nullish values because of the component that uses it: PageHeaderWithActions

Screen Shot 2022-09-23 at 4 45 18 PM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


it('renders a Header when passed in', async () => {
await waitForRender();

expect(
screen.getByRole('textbox', {
name: /dataset name/i,
}),
).toBeVisible();
expect(screen.getByTestId('editable-title')).toBeVisible();
});

it('renders a LeftPanel when passed in', async () => {
Expand Down