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

fix: Create dataset polish/bug fix #22262

Merged
merged 8 commits into from
Dec 20, 2022
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
41 changes: 23 additions & 18 deletions superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,28 @@ const collapseStyles = (theme: SupersetTheme) => css`
}
`;

const NO_DATABASES_MATCH_TITLE = t('No databases match your search');
const NO_DATABASES_AVAILABLE_TITLE = t('There are no databases available');
const MANAGE_YOUR_DATABASES_TEXT = t('Manage your databases');
const HERE_TEXT = t('here');

export const emptyStateComponent = (emptyResultsWithSearch: boolean) => (
Copy link
Member

Choose a reason for hiding this comment

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

If this is going to be exported, I think that we should move it out of the SQlEditorLeftBar? Where are the other empty state components stored? THat's where it should be.

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 thinking! Done in this commit.

<EmptyStateSmall
image="empty.svg"
title={
emptyResultsWithSearch
? NO_DATABASES_MATCH_TITLE
: NO_DATABASES_AVAILABLE_TITLE
}
description={
<p>
{MANAGE_YOUR_DATABASES_TEXT}{' '}
<a href="/databaseview/list">{HERE_TEXT}</a>
</p>
}
/>
);

const SqlEditorLeftBar = ({
database,
queryEditorId,
Expand Down Expand Up @@ -197,23 +219,6 @@ const SqlEditorLeftBar = ({
const shouldShowReset = window.location.search === '?reset=1';
const tableMetaDataHeight = height - 130; // 130 is the height of the selects above

const emptyStateComponent = (
<EmptyStateSmall
image="empty.svg"
title={
emptyResultsWithSearch
? t('No databases match your search')
: t('There are no databases available')
}
description={
<p>
{t('Manage your databases')}{' '}
<a href="/databaseview/list">{t('here')}</a>
</p>
}
/>
);

const handleSchemaChange = useCallback((schema: string) => {
if (queryEditor) {
dispatch(queryEditorSetSchema(queryEditor, schema));
Expand Down Expand Up @@ -248,7 +253,7 @@ const SqlEditorLeftBar = ({
<div data-test="sql-editor-left-bar" className="SqlEditorLeftBar">
<TableSelectorMultiple
onEmptyResults={onEmptyResults}
emptyState={emptyStateComponent}
emptyState={emptyStateComponent(emptyResultsWithSearch)}
database={userSelectedDb}
getDbList={handleDbList}
handleError={handleError}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ describe('AddDataset', () => {
const blankeStateImgs = screen.getAllByRole('img', { name: /empty/i });

// Header
expect(await screen.findByTestId('editable-title')).toBeVisible();
expect(await screen.findByText(/new dataset/i)).toBeVisible();
// Left panel
expect(blankeStateImgs[0]).toBeVisible();
// Footer
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import DatasetPanel, {
ALT_LOADING,
tableColumnDefinition,
COLUMN_TITLE,
} from './DatasetPanel';
} from 'src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel';
import { exampleColumns, exampleDataset } from './fixtures';
import {
SELECT_MESSAGE,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ const HALF = 0.5;
const MARGIN_MULTIPLIER = 3;

const StyledHeader = styled.div<StyledHeaderProps>`
${({ theme }) => `
position: ${(props: StyledHeaderProps) => props.position};
${({ theme, position }) => `
position: ${position};
margin: ${theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px
${theme.gridUnit * MARGIN_MULTIPLIER}px
${theme.gridUnit * MARGIN_MULTIPLIER}px
Expand Down Expand Up @@ -124,17 +124,27 @@ const StyledLoader = styled.div`
`}
`;

const TableContainer = styled.div`
const TableContainerWithBanner = styled.div`
${({ theme }) => `
position: relative;
margin: ${theme.gridUnit * MARGIN_MULTIPLIER}px;
margin-left: ${theme.gridUnit * (MARGIN_MULTIPLIER + 3)}px;
height: calc(100% - ${theme.gridUnit * 60}px);
overflow: auto;
`}
`;

const TableContainerWithoutBanner = styled.div`
${({ theme }) => `
position: relative;
margin: ${theme.gridUnit * MARGIN_MULTIPLIER}px;
margin-left: ${theme.gridUnit * (MARGIN_MULTIPLIER + 3)}px;
overflow: scroll;
height: calc(100% - ${theme.gridUnit * 36}px);
height: calc(100% - ${theme.gridUnit * 30}px);
overflow: auto;
`}
`;

const StyledTable = styled(Table)`
const TableScrollContainer = styled.div`
position: absolute;
left: 0;
top: 0;
Expand Down Expand Up @@ -253,6 +263,9 @@ const DatasetPanel = ({
const theme = useTheme();
const hasColumns = columnList?.length > 0 ?? false;
const datasetNames = datasets?.map(dataset => dataset.table_name);
const tableWithDataset = datasets?.find(
dataset => dataset.table_name === tableName,
);

let component;
let loader;
Expand All @@ -271,16 +284,33 @@ const DatasetPanel = ({
component = (
<>
<StyledTitle>{COLUMN_TITLE}</StyledTitle>
<TableContainer>
<StyledTable
loading={loading}
size={TableSize.SMALL}
columns={tableColumnDefinition}
data={columnList}
pageSizeOptions={pageSizeOptions}
defaultPageSize={10}
/>
</TableContainer>
{tableWithDataset ? (
<TableContainerWithBanner>
<TableScrollContainer>
<Table
loading={loading}
size={TableSize.SMALL}
columns={tableColumnDefinition}
data={columnList}
pageSizeOptions={pageSizeOptions}
defaultPageSize={25}
/>
</TableScrollContainer>
</TableContainerWithBanner>
) : (
<TableContainerWithoutBanner>
<TableScrollContainer>
<Table
loading={loading}
size={TableSize.SMALL}
columns={tableColumnDefinition}
data={columnList}
pageSizeOptions={pageSizeOptions}
defaultPageSize={25}
Copy link
Member

Choose a reason for hiding this comment

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

can we make this a constant at the top?

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! Done in this commit.

/>
</TableScrollContainer>
</TableContainerWithoutBanner>
)}
</>
);
} else {
Expand All @@ -299,9 +329,7 @@ const DatasetPanel = ({
{tableName && (
<>
{datasetNames?.includes(tableName) &&
renderExistingDatasetAlert(
datasets?.find(dataset => dataset.table_name === tableName),
)}
renderExistingDatasetAlert(tableWithDataset)}
<StyledHeader
position={
!loading && hasColumns ? EPosition.RELATIVE : EPosition.ABSOLUTE
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,33 +31,23 @@ describe('Header', () => {
test('renders a blank state Header', async () => {
await waitForRender();

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,
});
const datasetName = screen.getByText(/new dataset/i);

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

test('displays "New dataset" when a table is not selected', async () => {
await waitForRender();

const datasetName = screen.getByTestId('editable-title');
const datasetName = screen.getByText(/new dataset/i);
expect(datasetName.innerHTML).toBe(DEFAULT_TITLE);
});

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 datasetName = screen.getByTestId('editable-title');
const datasetName = screen.getByText(/testtable/i);

expect(datasetName.innerHTML).toBe('testTable');
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import { TooltipPlacement } from 'src/components/Tooltip';
import {
HeaderComponentStyles,
disabledSaveBtnStyles,
StyledCreateDatasetTitle,
} from 'src/views/CRUD/data/dataset/styles';
import {
DatasetActionType,
Expand Down Expand Up @@ -62,10 +63,12 @@ const renderOverlay = () => (
export default function Header({
setDataset,
title = DEFAULT_TITLE,
editing = false,
}: {
setDataset: React.Dispatch<DSReducerActionType>;
title?: string | null | undefined;
schema?: string | null | undefined;
editing?: boolean;
}) {
const editableTitleProps = {
title: title ?? DEFAULT_TITLE,
Expand All @@ -82,19 +85,25 @@ export default function Header({

return (
<HeaderComponentStyles>
<PageHeaderWithActions
editableTitleProps={editableTitleProps}
showTitlePanelItems={false}
showFaveStar={false}
faveStarProps={{ itemId: 1, saveFaveStar: () => {} }}
titlePanelAdditionalItems={<></>}
rightPanelAdditionalItems={renderDisabledSaveButton()}
additionalActionsMenu={renderOverlay()}
menuDropdownProps={{
disabled: true,
}}
tooltipProps={tooltipProps}
/>
{editing ? (
<PageHeaderWithActions
editableTitleProps={editableTitleProps}
showTitlePanelItems={false}
showFaveStar={false}
faveStarProps={{ itemId: 1, saveFaveStar: () => {} }}
titlePanelAdditionalItems={<></>}
rightPanelAdditionalItems={renderDisabledSaveButton()}
additionalActionsMenu={renderOverlay()}
menuDropdownProps={{
disabled: true,
}}
tooltipProps={tooltipProps}
/>
) : (
<StyledCreateDatasetTitle>
{title || DEFAULT_TITLE}
</StyledCreateDatasetTitle>
)}
</HeaderComponentStyles>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import DatabaseSelector, {
} from 'src/components/DatabaseSelector';
import { EmptyStateMedium } from 'src/components/EmptyState';
import { useToasts } from 'src/components/MessageToasts/withToasts';
import { emptyStateComponent } from 'src/SqlLab/components/SqlEditorLeftBar';
import { DatasetActionType } from '../types';

interface LeftPanelProps {
Expand All @@ -63,7 +64,7 @@ const LeftPanelStyle = styled.div`
}
.refresh {
position: absolute;
top: ${theme.gridUnit * 37.25}px;
top: ${theme.gridUnit * 38.75}px;
left: ${theme.gridUnit * 16.75}px;
span[role="button"]{
font-size: ${theme.gridUnit * 4.25}px;
Expand All @@ -87,6 +88,10 @@ const LeftPanelStyle = styled.div`
left: ${theme.gridUnit * 3.25}px;
right: 0;

.no-scrollbar {
margin-right: ${theme.gridUnit * 4}px;
}

.options {
cursor: pointer;
padding: ${theme.gridUnit * 1.75}px;
Expand All @@ -112,7 +117,7 @@ const LeftPanelStyle = styled.div`
}
form > span[aria-label="refresh"] {
position: absolute;
top: ${theme.gridUnit * 67.5}px;
top: ${theme.gridUnit * 69}px;
left: ${theme.gridUnit * 42.75}px;
font-size: ${theme.gridUnit * 4.25}px;
}
Expand All @@ -121,13 +126,13 @@ const LeftPanelStyle = styled.div`
}
.loading-container {
position: absolute;
top: 359px;
top: ${theme.gridUnit * 89.75}px;
left: 0;
right: 0;
text-align: center;
img {
width: ${theme.gridUnit * 20}px;
margin-bottom: 10px;
margin-bottom: ${theme.gridUnit * 2.5}px;
}
p {
color: ${theme.colors.grayscale.light1};
Expand Down Expand Up @@ -240,6 +245,15 @@ export default function LeftPanel({
const REFRESH_TABLES_TEXT = t('Refresh tables');
const SEARCH_TABLES_PLACEHOLDER_TEXT = t('Search tables');

const optionsList = document.getElementsByClassName('options-list');
const scrollableOptionsList =
optionsList[0]?.scrollHeight > optionsList[0]?.clientHeight;
const [emptyResultsWithSearch, setEmptyResultsWithSearch] = useState(false);

const onEmptyResults = (searchText?: string) => {
setEmptyResultsWithSearch(!!searchText);
};

return (
<LeftPanelStyle>
<p className="section-title db-schema">
Expand All @@ -249,6 +263,8 @@ export default function LeftPanel({
handleError={addDangerToast}
onDbChange={setDatabase}
onSchemaChange={setSchema}
emptyState={emptyStateComponent(emptyResultsWithSearch)}
onEmptyResults={onEmptyResults}
/>
{loadTables && !refresh && Loader(TABLE_LOADING_TEXT)}
{schema && !loadTables && !tableOptions.length && !searchVal && (
Expand Down Expand Up @@ -291,7 +307,13 @@ export default function LeftPanel({
filteredOptions.map((option, i) => (
<div
className={
selectedTable === i ? 'options-highlighted' : 'options'
selectedTable === i
? scrollableOptionsList
? 'options-highlighted'
: 'options-highlighted no-scrollbar'
: scrollableOptionsList
? 'options'
: 'options no-scrollbar'
}
key={i}
role="button"
Expand All @@ -308,7 +330,7 @@ export default function LeftPanel({
}
iconSize="m"
css={css`
margin-right: ${theme.gridUnit * 6}px;
margin-right: ${theme.gridUnit * 2}px;
`}
/>
)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ describe('DatasetLayout', () => {
it('renders a Header when passed in', async () => {
await waitForRender();

expect(screen.getByTestId('editable-title')).toBeVisible();
expect(screen.getByText(/new dataset/i)).toBeVisible();
});

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