-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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: add empty states to sqlab editor and select #19598
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pkdotson The Select has a property called notFoundContent
that can be used to implement this behavior. Keep in mind that if a user searches for a database and none is found, the same screen will appear. So I recommend a more generic message like "No databases found" instead of "There are no database available"
@pkdotson Tip: If you leave a blank line before the video URL in the PR description, Github will show the video instead of the URL.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pkdotson The DatabaseSelector
is a generic component meant to be used by multiple modules. That's why it's inside src/components
. You shouldn't add SQL Lab specific logic inside this component like you're doing when interacting with Redux. All this logic should be encapsulated in a SQL Lab component that uses the DatabaseSelector
and pass a notFoundContent
property when applicable.
But this component is only being used in Sqllab and used in changing the database. It makes sense for the action to happen here as this where that action is happening. |
This component is also used by Explore and the dataset listing. |
superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #19598 +/- ##
=======================================
Coverage 66.51% 66.51%
=======================================
Files 1684 1687 +3
Lines 64560 64622 +62
Branches 6625 6646 +21
=======================================
+ Hits 42939 42982 +43
- Misses 19926 19940 +14
- Partials 1695 1700 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
/testenv up |
@jinghua-qa Ephemeral environment spinning up at http://34.222.138.157:8080. Credentials are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for addressing the comments @pkdotson! I added more non-blocking suggestions regarding the names of the variables.
Here's the Git diff to fix the DatabaseSelector.test.tsx
diff --git a/superset-frontend/src/components/DatabaseSelector/DatabaseSelector.test.tsx b/superset-frontend/src/components/DatabaseSelector/DatabaseSelector.test.tsx
index de152cacb..272249b54 100644
--- a/superset-frontend/src/components/DatabaseSelector/DatabaseSelector.test.tsx
+++ b/superset-frontend/src/components/DatabaseSelector/DatabaseSelector.test.tsx
@@ -21,12 +21,12 @@ import React from 'react';
import { render, screen, waitFor } from 'spec/helpers/testing-library';
import { SupersetClient } from '@superset-ui/core';
import userEvent from '@testing-library/user-event';
-import DatabaseSelector from '.';
+import DatabaseSelector, { DatabaseSelectorProps } from '.';
import { EmptyStateSmall } from '../EmptyState';
const SupersetClientGet = jest.spyOn(SupersetClient, 'get');
-const createProps = () => ({
+const createProps = (): DatabaseSelectorProps => ({
db: {
id: 1,
database_name: 'test',
@@ -39,12 +39,10 @@ const createProps = () => ({
schema: undefined,
sqlLabMode: true,
getDbList: jest.fn(),
- getTableList: jest.fn(),
handleError: jest.fn(),
onDbChange: jest.fn(),
onSchemaChange: jest.fn(),
onSchemasLoad: jest.fn(),
- onUpdate: jest.fn(),
});
beforeEach(() => {
@@ -192,12 +190,10 @@ test('Refresh should work', async () => {
await waitFor(() => {
expect(SupersetClientGet).toBeCalledTimes(2);
expect(props.getDbList).toBeCalledTimes(0);
- expect(props.getTableList).toBeCalledTimes(0);
expect(props.handleError).toBeCalledTimes(0);
expect(props.onDbChange).toBeCalledTimes(0);
expect(props.onSchemaChange).toBeCalledTimes(0);
expect(props.onSchemasLoad).toBeCalledTimes(0);
- expect(props.onUpdate).toBeCalledTimes(0);
});
userEvent.click(screen.getByRole('button', { name: 'refresh' }));
@@ -205,12 +201,10 @@ test('Refresh should work', async () => {
await waitFor(() => {
expect(SupersetClientGet).toBeCalledTimes(3);
expect(props.getDbList).toBeCalledTimes(1);
- expect(props.getTableList).toBeCalledTimes(0);
expect(props.handleError).toBeCalledTimes(0);
expect(props.onDbChange).toBeCalledTimes(0);
expect(props.onSchemaChange).toBeCalledTimes(0);
expect(props.onSchemasLoad).toBeCalledTimes(2);
- expect(props.onUpdate).toBeCalledTimes(0);
});
});
@@ -226,17 +220,15 @@ test('Should database select display options', async () => {
});
test('should show empty state if there are no options', async () => {
+ SupersetClientGet.mockImplementation(
+ async () => ({ json: { result: [] } } as any),
+ );
const props = createProps();
- // @ts-ignore
- props.db = null;
- // @ts-ignore
- props.results = [];
- const { getByAltText } = await render(
+ render(
<DatabaseSelector
{...props}
- emptyState={
- <EmptyStateSmall title="testing" description="testing" image="" />
- }
+ db={undefined}
+ emptyState={<EmptyStateSmall title="empty" image="" />}
/>,
{ useRedux: true },
);
@@ -244,7 +236,7 @@ test('should show empty state if there are no options', async () => {
name: 'Select database or type database name',
});
userEvent.click(select);
- const emptystate = getByAltText('empty');
+ const emptystate = await screen.findByText('empty');
expect(emptystate).toBeInTheDocument();
expect(screen.queryByText('test-mysql')).not.toBeInTheDocument();
});
superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx
Outdated
Show resolved
Hide resolved
/testenv up |
@yousoph Ephemeral environment spinning up at http://34.222.112.28:8080. Credentials are |
….tsx Co-authored-by: Michael S. Molina <[email protected]>
….tsx Co-authored-by: Michael S. Molina <[email protected]>
….tsx Co-authored-by: Michael S. Molina <[email protected]>
….tsx Co-authored-by: Michael S. Molina <[email protected]>
/testenv up |
@yousoph Ephemeral environment spinning up at http://34.219.6.87:8080. Credentials are |
superset-frontend/src/components/DatabaseSelector/DatabaseSelector.test.tsx
Outdated
Show resolved
Hide resolved
Ephemeral environment shutdown and build artifacts deleted. |
* feat: add empty states to sqlab editor and select * add suggestions and test * update type * lint fix and add suggestions * fix typo * run lint * remove unused code * fix test * remove redux for propagation and other suggestions * add t * lint * fix text and remove code * ts and fix t in p * fix spelling * remove unused prop * add fn to prop change state * remove unused code * remove unused types * update code and test * fix lint * fix ts * update ts * add type export and fix test * Update superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx Co-authored-by: Michael S. Molina <[email protected]> * Update superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx Co-authored-by: Michael S. Molina <[email protected]> * Update superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx Co-authored-by: Michael S. Molina <[email protected]> * Update superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx Co-authored-by: Michael S. Molina <[email protected]> * remove handlerror and unused code Co-authored-by: Michael S. Molina <[email protected]>
🏷️ preset:2022.15 |
* feat: add empty states to sqlab editor and select * add suggestions and test * update type * lint fix and add suggestions * fix typo * run lint * remove unused code * fix test * remove redux for propagation and other suggestions * add t * lint * fix text and remove code * ts and fix t in p * fix spelling * remove unused prop * add fn to prop change state * remove unused code * remove unused types * update code and test * fix lint * fix ts * update ts * add type export and fix test * Update superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx Co-authored-by: Michael S. Molina <[email protected]> * Update superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx Co-authored-by: Michael S. Molina <[email protected]> * Update superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx Co-authored-by: Michael S. Molina <[email protected]> * Update superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx Co-authored-by: Michael S. Molina <[email protected]> * remove handlerror and unused code Co-authored-by: Michael S. Molina <[email protected]>
SUMMARY
This pr creates an empty state for the dropdown for the table selector and query editor when there is no database selected.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
after
Screen.Recording.2022-04-07.at.11.41.56.AM.mov
TESTING INSTRUCTIONS
Go to sql editor. If there is no db selected to query there should be an empty state for the query editor. If there is no db connected to db select dropdown should show empty state with link to foward user to db listview.
ADDITIONAL INFORMATION