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

chore(explore): Visual updates to explore datasource panel #20317

Closed
wants to merge 5 commits into from
Closed

chore(explore): Visual updates to explore datasource panel #20317

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bd3b6a9
Visual updates to explore datasource panel
lyndsiWilliams Jun 8, 2022
64f4181
Temporarily make Query icon visible
lyndsiWilliams Jun 8, 2022
7c07534
Remove Query icon visibility
lyndsiWilliams Jun 8, 2022
6fab57f
Removed isValidDatasourceType check
lyndsiWilliams Jun 8, 2022
31dfdd2
Adjusts conditional logic approach to be extensible for additional ty…
eric-briscoe Jun 13, 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
13 changes: 4 additions & 9 deletions superset-frontend/src/explore/components/ExploreViewContainer/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,10 +126,10 @@ const ExplorePanelContainer = styled.div`
position: relative;
display: flex;
flex-direction: row;
padding: 0 ${theme.gridUnit * 4}px;
padding: 0 ${theme.gridUnit * 2}px 0 ${theme.gridUnit * 4}px;
justify-content: space-between;
.horizontal-text {
font-size: ${theme.typography.sizes.s}px;
font-size: ${theme.typography.sizes.m}px;
}
}
.no-show {
Expand All @@ -145,7 +145,7 @@ const ExplorePanelContainer = styled.div`
padding: ${theme.gridUnit * 2}px;
width: ${theme.gridUnit * 8}px;
}
.callpase-icon > svg {
.collapse-icon > svg {
color: ${theme.colors.primary.base};
}
`};
Expand Down Expand Up @@ -603,7 +603,7 @@ function ExploreViewContainer(props) {
}
>
<div className="title-container">
<span className="horizontal-text">{t('Dataset')}</span>
<span className="horizontal-text">{t('Chart Source')}</span>
<span
role="button"
tabIndex={0}
Expand Down Expand Up @@ -642,11 +642,6 @@ function ExploreViewContainer(props) {
/>
</Tooltip>
</span>
<Icons.DatasetPhysical
css={{ marginTop: theme.gridUnit * 2 }}
iconSize="l"
iconColor={theme.colors.grayscale.base}
/>
</div>
) : null}
<Resizable
Expand Down
34 changes: 23 additions & 11 deletions superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
*/
import React from 'react';
import PropTypes from 'prop-types';
import { t, styled, withTheme } from '@superset-ui/core';
import { t, styled, withTheme, isValidDatasourceType } from '@superset-ui/core';
import { getUrlParam } from 'src/utils/urlUtils';

import { AntdDropdown } from 'src/components';
Expand Down Expand Up @@ -97,7 +97,7 @@ const Styles = styled.div`
white-space: nowrap;
overflow: hidden;
}
.dataset-svg {
.datasource-svg {
margin-right: ${({ theme }) => 2 * theme.gridUnit}px;
flex: none;
}
Expand Down Expand Up @@ -244,9 +244,23 @@ class DatasourceControl extends React.PureComponent {
return (
<Styles data-test="datasource-control" className="DatasourceControl">
<div className="data-container">
<Icons.DatasetPhysical className="dataset-svg" />
{/* Add a tooltip only for long dataset names */}
{!isMissingDatasource && datasource.name.length > 25 ? (
{isValidDatasourceType ? (
<Icons.ConsoleSqlOutlined className="datasource-svg" />
) : (
<Icons.DatasetPhysical className="datasource-svg" />
)}
{isValidDatasourceType ? (
Copy link
Member

@hughhhh hughhhh Jun 8, 2022 •
edited
Loading

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

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 changed this to be a strict check for just Query in this commit 😁

/* Add a tooltip only for long dataset names */
!isMissingDatasource && datasource.sql.length > 25 ? (
Copy link
Member

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?

Copy link
Member Author

@lyndsiWilliams lyndsiWilliams Jun 8, 2022 •
edited
Loading

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.

Copy link
Member

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?

<Tooltip title={datasource.sql}>
<span className="title-select">{datasource.sql}</span>
</Tooltip>
) : (
<span title={datasource.sql} className="title-select">
{datasource.sql}
</span>
)
) : !isMissingDatasource && datasource.name.length > 25 ? (
<Tooltip title={datasource.name}>
<span className="title-select">{datasource.name}</span>
</Tooltip>
Expand All @@ -268,12 +282,10 @@ class DatasourceControl extends React.PureComponent {
trigger={['click']}
data-test="datasource-menu"
>
<Tooltip title={t('More dataset related options')}>
<Icons.MoreVert
className="datasource-modal-trigger"
data-test="datasource-menu-trigger"
/>
</Tooltip>
<Icons.MoreVert
className="datasource-modal-trigger"
data-test="datasource-menu-trigger"
/>
</AntdDropdown>
</div>
{/* missing dataset */}
Expand Down