-
Notifications
You must be signed in to change notification settings - Fork 14k
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(explore): Dataset Panel Options when Source = Query II #20299
feat(explore): Dataset Panel Options when Source = Query II #20299
Conversation
</Menu> | ||
); | ||
|
||
const datasourceTypeCheck = |
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.
do we have this somewhere else? Also the current dataset is SlTable.. but I think I remember seeing us using Dataset?
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.
Oh whoops, good catch! This already exists as isValidDatasourceType
in DatasourcePanel/index.tsx
here: https://github.com/apache/superset/blob/master/superset-frontend/src/explore/components/DatasourcePanel/index.tsx#L310-L314
So I'll move/export/use that one instead.
But it does include SlTable, as well as Dataset. I think it was set up that way to check for any of the DatasourceTypes (minus table and druid). Should I remove it?
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.
Ok, since this still lives in two places, can we dry it up a bit with something like
Object.values(DatasourceType).includes(datasource.type)
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.
nm, I see that you removed it in the other component. Still possibly a good way to make it more succinct and potentially remove the import.
/testenv up |
@lyndsiWilliams Ephemeral environment spinning up at http://34.209.46.24:8080. Credentials are |
Codecov Report
@@ Coverage Diff @@
## master #20299 +/- ##
==========================================
- Coverage 66.61% 66.60% -0.02%
==========================================
Files 1733 1734 +1
Lines 64953 65007 +54
Branches 6858 6870 +12
==========================================
+ Hits 43268 43296 +28
- Misses 19929 19953 +24
- Partials 1756 1758 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
overlay={datasourceMenu} | ||
overlay={ | ||
isValidDatasourceType(datasource.type) | ||
? queryDatasourceMenu |
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.
is it just showing the query now for testing?
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.
This only shows the query dropdown if the datasource is a query, it should not be showing at this point.
@@ -327,7 +321,7 @@ export default function DataSourcePanel({ | |||
placeholder={t('Search Metrics & Columns')} | |||
/> | |||
<div className="field-selections"> | |||
{isValidDatasourceType && showInfoboxCheck() && ( | |||
{isValidDatasourceType(datasource.type) && showInfoboxCheck() && ( |
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.
is this supposed to show for only a certain type too?
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!
Ephemeral environment shutdown and build artifacts deleted. |
SUMMARY
This adds a different dataset panel dropdown when a chart's datasource is a query.
SCREENSHOTS / ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION