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

fix: DB connection modal connect bug #21299

Merged
merged 7 commits into from
Sep 2, 2022
Merged

fix: DB connection modal connect bug #21299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1961582
dataset exists error
AAfghahi Aug 29, 2022
d4255b4
Merge branch 'master' of https://github.com/apache/superset
AAfghahi Aug 31, 2022
dc65632
Merge branch 'master' of https://github.com/apache/superset
AAfghahi Sep 1, 2022
e74b906
Merge branch 'master' of https://github.com/apache/superset
AAfghahi Sep 2, 2022
ad649c5
fix connect issue
AAfghahi Sep 1, 2022
192f52d
new design
AAfghahi Sep 1, 2022
df47f96
added testing
AAfghahi Sep 1, 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
3 changes: 3 additions & 0 deletions superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ describe('DatabaseModal', () => {
hidden: true,
});

const footer = document.getElementsByClassName('ant-modal-footer');
// ---------- TODO (lyndsiWilliams): Selector options, can't seem to get these to render properly.

// renderAvailableSelector() => <Alert> - Supported databases alert
Expand Down Expand Up @@ -312,6 +313,8 @@ describe('DatabaseModal', () => {
visibleComponents.forEach(component => {
expect(component).toBeVisible();
});
// there should be a footer but it should not have any buttons in it
expect(footer[0]).toBeEmptyDOMElement();
// This is how many preferred databases are rendered
expect(preferredDbIcon).toHaveLength(4);
});
Expand Down
10 changes: 6 additions & 4 deletions superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -847,7 +847,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({

const renderModalFooter = () => {
if (db) {
// if db show back + connenct
// if db show back + connect
if (!hasConnectedDb || editNewDb) {
return (
<>
Expand Down Expand Up @@ -901,7 +901,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
);
}

return [];
return <></>;
Copy link
Member

Choose a reason for hiding this comment

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

just a style nit, but most times that I've seen components render nothing, it would be a return null here.

Copy link
Member

Choose a reason for hiding this comment

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

not a blocker.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok let me test to make sure that this still renders the expected behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok no, that would render the default footer component because of the logic here:
https://github.com/apache/superset/blob/master/superset-frontend/src/components/Modal/Modal.tsx#L256

eschutho marked this conversation as resolved.
Show resolved Hide resolved
};

const renderEditModalFooter = (db: Partial<DatabaseObject> | null) => (
Expand Down Expand Up @@ -1302,7 +1302,9 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
</Modal>
);
}

const modalFooter = isEditMode
? renderEditModalFooter(db)
: renderModalFooter();
return useTabLayout ? (
<Modal
css={(theme: SupersetTheme) => [
Expand All @@ -1323,7 +1325,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
title={
<h4>{isEditMode ? t('Edit database') : t('Connect a database')}</h4>
}
footer={isEditMode ? renderEditModalFooter(db) : renderModalFooter()}
footer={modalFooter}
>
<StyledStickyHeader>
<TabHeader>
Expand Down
3 changes: 2 additions & 1 deletion superset-frontend/src/views/CRUD/data/database/DatabaseModal/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import Button from 'src/components/Button';
const CTAS_CVAS_SCHEMA_FORM_HEIGHT = 108;
const EXPOSE_IN_SQLLAB_FORM_HEIGHT = CTAS_CVAS_SCHEMA_FORM_HEIGHT + 153;
const EXPOSE_ALL_FORM_HEIGHT = EXPOSE_IN_SQLLAB_FORM_HEIGHT + 102;
const MODAL_BODY_HEIGHT = 180.5;

const anticonHeight = 12;

Expand Down Expand Up @@ -156,7 +157,7 @@ export const antDModalStyles = (theme: SupersetTheme) => css`
}
.ant-modal-body {
height: ${theme.gridUnit * 180.5}px;
height: ${theme.gridUnit * MODAL_BODY_HEIGHT}px;
}
.ant-modal-footer {
Expand Down