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.

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

feat: Create dataset header component #21189

Merged
merged 6 commits into from
Sep 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import React, { ReactNode, ReactElement } from 'react';
import { css, SupersetTheme, t, useTheme } from '@superset-ui/core';
import { AntdDropdown, AntdDropdownProps } from 'src/components';
import { TooltipPlacement } from 'src/components/Tooltip';
import {
DynamicEditableTitle,
DynamicEditableTitleProps,
Expand Down Expand Up @@ -112,6 +113,10 @@ export type PageHeaderWithActionsProps = {
rightPanelAdditionalItems: ReactNode;
additionalActionsMenu: ReactElement;
menuDropdownProps: Omit<AntdDropdownProps, 'overlay'>;
tooltipProps?: {
text?: string;
placement?: TooltipPlacement;
};
};

export const PageHeaderWithActions = ({
Expand All @@ -124,6 +129,7 @@ export const PageHeaderWithActions = ({
rightPanelAdditionalItems,
additionalActionsMenu,
menuDropdownProps,
tooltipProps,
}: PageHeaderWithActionsProps) => {
const theme = useTheme();
return (
Expand Down Expand Up @@ -152,6 +158,8 @@ export const PageHeaderWithActions = ({
css={menuTriggerStyles}
buttonStyle="tertiary"
aria-label={t('Menu actions trigger')}
tooltip={tooltipProps?.text}
placement={tooltipProps?.placement}
data-test="actions-trigger"
>
<Icons.MoreHoriz
Expand Down
7 changes: 6 additions & 1 deletion superset-frontend/src/components/Tooltip/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,14 @@
import React from 'react';
import { useTheme, css } from '@superset-ui/core';
import { Tooltip as AntdTooltip } from 'antd';
import { TooltipProps } from 'antd/lib/tooltip';
import {
TooltipProps,
TooltipPlacement as AntdTooltipPlacement,
} from 'antd/lib/tooltip';
import { Global } from '@emotion/react';

export type TooltipPlacement = AntdTooltipPlacement;

export const Tooltip = (props: TooltipProps) => {
const theme = useTheme();
return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,12 @@ describe('AddDataset', () => {

const blankeStateImgs = screen.getAllByRole('img', { name: /empty/i });

expect(await screen.findByText(/header/i)).toBeInTheDocument();
// Header
expect(screen.getByText(/header/i)).toBeVisible();
expect(
await screen.findByRole('textbox', {
name: /dataset name/i,
}),
).toBeVisible();
// Left panel
expect(blankeStateImgs[0]).toBeVisible();
// Footer
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,64 @@
* specific language governing permissions and limitations
* under the License.
*/
import userEvent from '@testing-library/user-event';
import React from 'react';
import { render, screen } from 'spec/helpers/testing-library';
import { render, screen, waitFor } from 'spec/helpers/testing-library';
import Header from 'src/views/CRUD/data/dataset/AddDataset/Header';

describe('Header', () => {
it('renders a blank state Header', () => {
render(<Header />);
const mockSetDataset = jest.fn();

expect(screen.getByText(/header/i)).toBeVisible();
const waitForRender = (datasetName: string) =>
waitFor(() =>
render(<Header setDataset={mockSetDataset} datasetName={datasetName} />),
);

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

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

expect(datasetNameTextbox).toBeVisible();
expect(saveButton).toBeVisible();
expect(saveButton).toBeDisabled();
expect(menuButton).toBeVisible();
expect(menuButton).toBeDisabled();
Copy link
Member

Choose a reason for hiding this comment

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

should there be a test for when someone passes in a name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can do! Added in this commit.

});

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,
});

// 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');
});

it('passes an existing dataset title into the dataset name textbox', async () => {
await waitForRender('Existing Dataset Name');

const datasetNameTextbox = screen.getByRole('textbox', {
name: /dataset name/i,
});

expect(datasetNameTextbox).toHaveDisplayValue('Existing Dataset Name');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,81 @@
* under the License.
*/
import React from 'react';
import { t } from '@superset-ui/core';
import { PageHeaderWithActions } from 'src/components/PageHeaderWithActions';
import Button from 'src/components/Button';
import Icons from 'src/components/Icons';
import { Menu } from 'src/components/Menu';
import { TooltipPlacement } from 'src/components/Tooltip';
import {
HeaderComponentStyles,
disabledSaveBtnStyles,
} from 'src/views/CRUD/data/dataset/styles';
import {
DatasetActionType,
DSReducerActionType,
} from 'src/views/CRUD/data/dataset/AddDataset/types';

export default function Header() {
return <div>Header</div>;
const tooltipProps: { text: string; placement: TooltipPlacement } = {
text: t('Select a database table and create dataset'),
placement: 'bottomRight',
};

const renderDisabledSaveButton = () => (
<Button
buttonStyle="primary"
tooltip={tooltipProps?.text}
placement={tooltipProps?.placement}
disabled
css={disabledSaveBtnStyles}
>
<Icons.Save iconSize="m" />
{t('Save')}
</Button>
);

const renderOverlay = () => (
<Menu>
<Menu.Item>{t('Settings')}</Menu.Item>
<Menu.Item>{t('Delete')}</Menu.Item>
</Menu>
);

export default function Header({
setDataset,
datasetName,
}: {
setDataset: React.Dispatch<DSReducerActionType>;
datasetName: string;
}) {
const editableTitleProps = {
title: datasetName,
placeholder: t('Add the name of the dataset'),
onSave: (newDatasetName: string) => {
setDataset({
type: DatasetActionType.changeDataset,
payload: { name: 'dataset_name', value: newDatasetName },
});
},
canEdit: true,
label: t('dataset name'),
};

return (
Copy link
Member

Choose a reason for hiding this comment

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

hey, quick question in regards to the save button. What happens, currently, when I press save? I believe that we have a state action that is supposed to put the name into state. Does it currently do that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, I have the data properly flowing now in this commit.

<HeaderComponentStyles>
<PageHeaderWithActions
editableTitleProps={editableTitleProps}
showTitlePanelItems={false}
showFaveStar={false}
faveStarProps={{ itemId: 1, saveFaveStar: () => {} }}
titlePanelAdditionalItems={<></>}
rightPanelAdditionalItems={renderDisabledSaveButton()}
additionalActionsMenu={renderOverlay()}
menuDropdownProps={{
disabled: true,
}}
tooltipProps={tooltipProps}
/>
</HeaderComponentStyles>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ export default function AddDataset() {
Reducer<Partial<DatasetObject> | null, DSReducerActionType>
>(datasetReducer, null);

const HeaderComponent = () => (
<Header setDataset={setDataset} datasetName={dataset?.dataset_name ?? ''} />
);

const LeftPanelComponent = () => (
<LeftPanel
setDataset={setDataset}
Expand All @@ -80,7 +84,7 @@ export default function AddDataset() {

return (
<DatasetLayout
header={Header()}
header={HeaderComponent()}
leftPanel={LeftPanelComponent()}
datasetPanel={DatasetPanel()}
footer={Footer()}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export interface DatasetObject {
table_name?: string | null;
}

interface DatasetReducerPayloadType {
export interface DatasetReducerPayloadType {
name: string;
value?: string;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/
import React from 'react';
import { render, screen } from 'spec/helpers/testing-library';
import { render, screen, waitFor } from 'spec/helpers/testing-library';
import DatasetLayout from 'src/views/CRUD/data/dataset/DatasetLayout';
import Header from 'src/views/CRUD/data/dataset/AddDataset/Header';
import LeftPanel from 'src/views/CRUD/data/dataset/AddDataset/LeftPanel';
Expand All @@ -33,10 +33,21 @@ describe('DatasetLayout', () => {
expect(layoutWrapper).toHaveTextContent('');
});

it('renders a Header when passed in', () => {
render(<DatasetLayout header={Header()} />);
const mockSetDataset = jest.fn();

expect(screen.getByText(/header/i)).toBeVisible();
const waitForRender = () =>
waitFor(() =>
render(<Header setDataset={mockSetDataset} datasetName="" />),
Copy link
Member

@hughhhh hughhhh Sep 8, 2022

Choose a reason for hiding this comment

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

add a test to make sure if datasetName is set that value of the textbox renders

Copy link
Member Author

Choose a reason for hiding this comment

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

);

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

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

it('renders a LeftPanel when passed in', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,11 @@ import {
OuterRow,
PanelRow,
FooterRow,
StyledHeader,
StyledLeftPanel,
StyledDatasetPanel,
StyledRightPanel,
StyledFooter,
StyledLayoutHeader,
StyledLayoutLeftPanel,
StyledLayoutDatasetPanel,
StyledLayoutRightPanel,
StyledLayoutFooter,
} from 'src/views/CRUD/data/dataset/styles';

interface DatasetLayoutProps {
Expand All @@ -48,22 +48,28 @@ export default function DatasetLayout({
}: DatasetLayoutProps) {
return (
<StyledLayoutWrapper data-test="dataset-layout-wrapper">
{header && <StyledHeader>{header}</StyledHeader>}
{header && <StyledLayoutHeader>{header}</StyledLayoutHeader>}
<OuterRow>
<LeftColumn>
{leftPanel && <StyledLeftPanel>{leftPanel}</StyledLeftPanel>}
{leftPanel && (
<StyledLayoutLeftPanel>{leftPanel}</StyledLayoutLeftPanel>
)}
</LeftColumn>

<RightColumn>
<PanelRow>
{datasetPanel && (
<StyledDatasetPanel>{datasetPanel}</StyledDatasetPanel>
<StyledLayoutDatasetPanel>
{datasetPanel}
</StyledLayoutDatasetPanel>
)}
{rightPanel && (
<StyledLayoutRightPanel>{rightPanel}</StyledLayoutRightPanel>
)}
{rightPanel && <StyledRightPanel>{rightPanel}</StyledRightPanel>}
</PanelRow>

<FooterRow>
{footer && <StyledFooter>{footer}</StyledFooter>}
{footer && <StyledLayoutFooter>{footer}</StyledLayoutFooter>}
</FooterRow>
</RightColumn>
</OuterRow>
Expand Down
Loading