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: Added error alert for DB connection Modal #15242

Merged
merged 22 commits into from
Jun 22, 2021
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
dbbf58c
fix: add icons (#15122)
AAfghahi Jun 11, 2021
e5b66dc
Merge branch 'pexdax/db-connection-ui' of ssh://github.com/apache/sup…
AAfghahi Jun 11, 2021
03d3326
Merge branch 'pexdax/db-connection-ui' of ssh://github.com/apache/sup…
AAfghahi Jun 15, 2021
5cfe664
Merge branch 'pexdax/db-connection-ui' of ssh://github.com/apache/sup…
AAfghahi Jun 15, 2021
7a7f39b
Merge branch 'pexdax/db-connection-ui' of ssh://github.com/apache/sup…
AAfghahi Jun 16, 2021
54965a1
Merge branch 'pexdax/db-connection-ui' of ssh://github.com/apache/sup…
AAfghahi Jun 16, 2021
9a2a58e
Merge branch 'pexdax/db-connection-ui' of ssh://github.com/apache/sup…
AAfghahi Jun 16, 2021
be0408b
Merge branch 'pexdax/db-connection-ui' of ssh://github.com/apache/sup…
AAfghahi Jun 16, 2021
be5ea73
Merge branch 'pexdax/db-connection-ui' of ssh://github.com/apache/sup…
AAfghahi Jun 16, 2021
a7425e4
Merge branch 'pexdax/db-connection-ui' of ssh://github.com/apache/sup…
AAfghahi Jun 16, 2021
8a8998e
Merge branch 'pexdax/db-connection-ui' of ssh://github.com/apache/sup…
AAfghahi Jun 17, 2021
fc31ea2
Merge branch 'pexdax/db-connection-ui' of ssh://github.com/apache/sup…
AAfghahi Jun 17, 2021
8a4ff62
spinner
AAfghahi Jun 17, 2021
34e8de1
Merge branch 'pexdax/db-connection-ui' of ssh://github.com/apache/sup…
AAfghahi Jun 17, 2021
cd6744f
Merge branch 'pexdax/db-connection-ui' of ssh://github.com/apache/sup…
AAfghahi Jun 17, 2021
4a0eefa
Merge branch 'pexdax/db-connection-ui' of ssh://github.com/apache/sup…
AAfghahi Jun 17, 2021
4a9f641
added errorAlert
AAfghahi Jun 17, 2021
0cee43f
conclude merge
AAfghahi Jun 18, 2021
6a88a41
added revisions
AAfghahi Jun 18, 2021
ed10ef3
current work
AAfghahi Jun 22, 2021
a8bc0f5
revisions
AAfghahi Jun 22, 2021
b7254f3
Merge branch 'pexdax/db-connection-ui' into arash/errorAlerts
AAfghahi Jun 22, 2021
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 @@ -54,6 +54,7 @@ import ExtraOptions from './ExtraOptions';
import SqlAlchemyForm from './SqlAlchemyForm';
import DatabaseConnectionForm from './DatabaseConnectionForm';
import {
antDErrorAlertStyles,
antDAlertStyles,
antDModalNoPaddingStyles,
antDModalStyles,
Expand Down Expand Up @@ -285,6 +286,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
const [tabKey, setTabKey] = useState<string>(DEFAULT_TAB_KEY);
const [availableDbs, getAvailableDbs] = useAvailableDatabases();
const [validationErrors, getValidation] = useDatabaseValidation();
const [hasErrors, setHasErrors] = useState<boolean>(false);
const [hasConnectedDb, setHasConnectedDb] = useState<boolean>(false);
const [dbName, setDbName] = useState('');
const [isLoading, setLoading] = useState<boolean>(false);
Expand Down Expand Up @@ -596,10 +598,32 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
}
}, [availableDbs]);

useEffect(() => {
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this useEffect, we can just key on validationErrors to show the alert
{
validationErrors &&
}

Copy link
Member Author

Choose a reason for hiding this comment

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

true good point.

if (validationErrors) {
setHasErrors(true);
} else {
setHasErrors(false);
}
}, [validationErrors]);
// Used as componentDidMount

const tabChange = (key: string) => {
setTabKey(key);
};

const errorAlert = () => {
const errors = validationErrors;
return (
<Alert
type="error"
css={(theme: SupersetTheme) => antDErrorAlertStyles(theme)}
message="Missing Required Fields"
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if you plan to put this into the v2, but I think that we're going to have to create a map between the server side error and the language that we want to show in these alerts. These are more descriptive and instructional and I don't think they will be or should be the same as what we are returning from the API.

Copy link
Member Author

Choose a reason for hiding this comment

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

I hadn't thought about a map, though I really like that.

No, I figured that language was prone to change, I wanted this in there to make sure that it looked ok, and so that I could style it.

description={errors ? errors.port : ''}
showIcon
/>
);
};

const isDynamic = (engine: string | undefined) =>
availableDbs?.databases.filter(
(DB: DatabaseObject) => DB.engine === engine,
Expand Down Expand Up @@ -763,6 +787,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
onChange(ActionType.extraEditorChange, payload);
}}
/>
{hasErrors && errorAlert}
</Tabs.TabPane>
</Tabs>
</Modal>
Expand Down Expand Up @@ -910,6 +935,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
Connect this database with a SQLAlchemy URI string instead
</Button>
{/* Step 2 */}
{hasErrors && errorAlert()}
</>
))}
</>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,27 @@ export const antDAlertStyles = (theme: SupersetTheme) => css`
}
`;

export const antDErrorAlertStyles = (theme: SupersetTheme) => css`
padding: ${theme.gridUnit * 4}px;
margin: ${theme.gridUnit * 8}px ${theme.gridUnit * 4}px;
.ant-alert-message {
color: ${theme.colors.info.dark2};
font-size: ${theme.typography.sizes.s + 1}px;
font-weight: bold;
}
.ant-alert-description {
color: ${theme.colors.info.dark2};
font-size: ${theme.typography.sizes.s + 1}px;
line-height: ${theme.gridUnit * 4}px;
.ant-alert-icon {
margin-right: ${theme.gridUnit * 2.5}px;
font-size: ${theme.typography.sizes.l + 1}px;
position: relative;
top: ${theme.gridUnit / 4}px;
}
}
`;

export const formHelperStyles = (theme: SupersetTheme) => css`
.required {
margin-left: ${theme.gridUnit / 2}px;
Expand Down