-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
chore(explore): Visual updates to explore datasource panel #20317
chore(explore): Visual updates to explore datasource panel #20317
Conversation
/testenv up |
@lyndsiWilliams Container image not yet published for this PR. Please try again when build is complete. |
@lyndsiWilliams Ephemeral environment creation failed. Please check the Actions logs for details. |
/testenv up |
@lyndsiWilliams Ephemeral environment spinning up at http://34.220.2.187:8080. Credentials are |
Codecov Report
@@ Coverage Diff @@
## master #20317 +/- ##
==========================================
- Coverage 66.61% 66.59% -0.02%
==========================================
Files 1733 1734 +1
Lines 64953 64998 +45
Branches 6858 6873 +15
==========================================
+ Hits 43268 43287 +19
- Misses 19929 19952 +23
- Partials 1756 1759 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
)} | ||
{isValidDatasourceType ? ( | ||
/* Add a tooltip only for long dataset names */ | ||
!isMissingDatasource && datasource.sql.length > 25 ? ( |
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.
curious here why datasource.sql.length have to be greater than 25?
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 was how it was initially set, I just added the query route. I'm also not sure why 25 haha. It's for long dataset names to get a tooltip.
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.
can we make this value a constant then?
) : ( | ||
<Icons.DatasetPhysical className="datasource-svg" /> | ||
)} | ||
{isValidDatasourceType ? ( |
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.
we need to check if the datasource.type is DatasourceType.Query
here, if so then show the image in sql as the name
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.
I changed this to be a strict check for just Query in this commit
😁
@@ -327,7 +321,7 @@ export default function DataSourcePanel({ | |||
placeholder={t('Search Metrics & Columns')} | |||
/> | |||
<div className="field-selections"> | |||
{isValidDatasourceType && showInfoboxCheck() && ( | |||
{datasource.type === DatasourceType.Query && 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.
I have added a type guard at here. We can define more type guard to handle different type of datasources.
<Icons.DatasetPhysical className="dataset-svg" /> | ||
{/* Add a tooltip only for long dataset names */} | ||
{!isMissingDatasource && datasource.name.length > 25 ? ( | ||
{datasource.type === DatasourceType.Query ? ( |
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.
same before
…pes from DatasourceTypes options
datasource.type === DatasourceType.SlTable || | ||
datasource.type === DatasourceType.SavedQuery || | ||
datasource.type === DatasourceType.Query; | ||
const dataSourceIsQuery = datasource?.type === DatasourceType.Query; |
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.
can we duck type this instead?
This got copied into this feature branch: #20281 |
Ephemeral environment shutdown and build artifacts deleted. |
SUMMARY
Made the following visual updates to the explore datasource panel:
SCREENSHOTS
When chart source is a Query:
When chart source is not a Query:
Collapsed panel (no longer has an icon):
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION