geido commented on code in PR #32112:
URL: https://github.com/apache/superset/pull/32112#discussion_r1960073525
##########
superset-frontend/src/components/Datasource/DatasourceModal.tsx:
##########
@@ -298,6 +301,11 @@ const DatasourceModal:
FunctionComponent<DatasourceModalProps> = ({
onHide={onHide}
title={
<span>
+ <Icons.EditOutlined
+ iconColor={theme.colors.grayscale.base}
+ css={{ margin: `auto ${theme.gridUnit * 2}px auto 0` }}
Review Comment:
Same as my other comments
##########
superset-frontend/src/components/DropdownSelectableIcon/index.tsx:
##########
@@ -0,0 +1,177 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { addAlpha, styled, useTheme } from '@superset-ui/core';
+import { FC, RefObject, useMemo, ReactNode, useState } from 'react';
+import Icons from 'src/components/Icons';
+import { DropdownButton } from 'src/components/DropdownButton';
+import { DropdownButtonProps } from 'antd/lib/dropdown';
+import { Menu, MenuProps } from 'src/components/Menu';
+
+const { SubMenu } = Menu;
+
+type SubMenuItemProps = { key: string; label: string | ReactNode };
+
+export interface DropDownSelectableProps extends Pick<MenuProps, 'onSelect'> {
+ ref?: RefObject<HTMLDivElement>;
+ icon: ReactNode;
+ info?: string;
+ menuItems: {
+ key: string;
+ label: string | ReactNode;
+ children?: SubMenuItemProps[];
+ divider?: boolean;
+ }[];
+ selectedKeys?: string[];
+}
+
+const StyledDropdownButton = styled(DropdownButton as FC<DropdownButtonProps>)`
+ button.ant-btn:first-of-type {
+ display: none;
+ }
+ > button.ant-btn:nth-of-type(2) {
+ display: inline-flex;
+ background-color: transparent !important;
+ height: unset;
+ padding: 0;
+ border: none;
+ width: auto !important;
+
+ .anticon {
+ line-height: 0;
+ }
+ &:after {
+ box-shadow: none !important;
+ }
+ }
+`;
+
+const StyledMenu = styled(Menu)`
+ ${({ theme }) => `
+ box-shadow:
+ 0 3px 6px -4px ${addAlpha(theme.colors.grayscale.dark2, 0.12)},
+ 0 6px 16px 0
+ ${addAlpha(theme.colors.grayscale.dark2, 0.08)},
+ 0 9px 28px 8px
+ ${addAlpha(theme.colors.grayscale.dark2, 0.05)};
+ .info {
+ font-size: ${theme.typography.sizes.s}px;
+ color: ${theme.colors.grayscale.base};
+ padding: ${theme.gridUnit}px ${theme.gridUnit * 3}px ${
+ theme.gridUnit
+ }px ${theme.gridUnit * 3}px;
+ }
+ .ant-dropdown-menu-item-selected {
Review Comment:
I think this might have been upgraded to
.`antd5-dropdown-menu-item-selected`. Just making sure
##########
superset-frontend/src/SqlLab/components/ResultSet/index.tsx:
##########
@@ -677,7 +688,8 @@ const ResultSet = ({
width: 100%;
overflow: hidden;
white-space: nowrap !important;
- text-overflow: ellipsis;
+ text-overflow: ellipsimport { CopyToClipboard } from
'.';
Review Comment:
You might want to check this
##########
superset-frontend/src/SqlLab/components/SqlEditorTabHeader/index.tsx:
##########
@@ -31,11 +37,17 @@ import {
toggleLeftBar,
} from 'src/SqlLab/actions/sqlLab';
import { QueryEditor, SqlLabRootState } from 'src/SqlLab/types';
-import TabStatusIcon from '../TabStatusIcon';
+import Icons, { IconType } from 'src/components/Icons';
const TabTitleWrapper = styled.div`
display: flex;
align-items: center;
+
+ [aria-label='check-circle'],
+ .status-icon {
+ margin: 0px;
+ margin-right: 0px;
Review Comment:
Do we need both?
##########
superset-frontend/src/components/Icons/index.tsx:
##########
@@ -24,66 +24,38 @@ import Icon from './Icon';
import IconType from './IconType';
const IconFileNames = [
- 'alert',
- 'alert_solid',
- 'alert_solid_small',
+ // to keep custom
Review Comment:
```suggestion
// to keep custom BEGIN
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Vertical.tsx:
##########
@@ -205,11 +195,18 @@ const VerticalFilterBar: FC<VerticalBarProps> = ({
role="button"
offset={offset}
>
- <StyledCollapseIcon
- {...getFilterBarTestId('expand-button')}
+ <Icons.VerticalAlignTopOutlined
iconSize="l"
+ css={{
Review Comment:
Here as well, lets use the string css pattern css={css``} for consistency
with the rest of the codebase
##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -1913,7 +1920,7 @@ const AlertReportModal:
FunctionComponent<AlertReportModalProps> = ({
))}
{
// Prohibit 'add notification method' button if only one present
- allowedNotificationMethodsCount > notificationSettings.length && (
+ true && (
Review Comment:
I think this needs to be reverted
##########
superset-frontend/src/components/Icons/index.tsx:
##########
@@ -92,78 +64,106 @@ const IconFileNames = [
'filter',
'filter_small',
'folder',
- 'full',
- 'function_x',
- 'gear',
'grid',
'image',
- 'import',
- 'info',
- 'info-solid',
- 'info_solid_small',
'join',
'keyboard',
- 'layers',
'lightbulb',
'line-chart-tile',
- 'link',
- 'list',
- 'list_view',
'location',
'lock_locked',
'lock_unlocked',
'map',
'message',
'minus',
'minus_solid',
- 'more_horiz',
- 'more_vert',
'move',
- 'nav_charts',
- 'nav_dashboard',
- 'nav_data',
- 'nav_explore',
- 'nav_home',
- 'nav_lab',
'note',
'offline',
- 'paperclip',
'pie-chart-tile',
'placeholder',
- 'plus',
- 'plus_large',
- 'plus_small',
- 'plus_solid',
'queued',
- 'refresh',
'running',
- 'save',
'sql',
- 'search',
- 'server',
- 'share',
'slack',
- 'sort_asc',
- 'sort_desc',
- 'sort',
- 'table',
- 'table-chart-tile',
- 'tag',
- 'trash',
'triangle_change',
- 'triangle_down',
- 'triangle_up',
'up-level',
'user',
- 'warning',
- 'warning_solid',
- 'x-large',
- 'x-small',
- 'tags',
'ballot',
'category',
'undo',
'redo',
+ // to remove
Review Comment:
```suggestion
```
##########
superset-frontend/src/components/Icons/index.tsx:
##########
@@ -24,66 +24,38 @@ import Icon from './Icon';
import IconType from './IconType';
const IconFileNames = [
- 'alert',
- 'alert_solid',
- 'alert_solid_small',
+ // to keep custom
+ 'binoculars',
+ 'certified',
+ 'circle_solid',
+ 'layers',
+ 'full',
+ 'sort_asc',
+ 'sort_desc',
+ 'sort',
+
Review Comment:
```suggestion
// to keep custom END
```
##########
superset-frontend/src/features/dashboards/DashboardCard.tsx:
##########
@@ -182,7 +195,10 @@ function DashboardCard({
)}
<Dropdown dropdownRender={() => menu} trigger={['hover', 'click']}>
<Button buttonSize="xsmall" type="link">
- <Icons.MoreVert iconColor={theme.colors.grayscale.base} />
+ <Icons.MoreOutlined
+ iconSize="xl"
+ iconColor={theme.colors.grayscale.base}
Review Comment:
Should this be the default color of the icons? I see this repeated all over
the places. I think it might make sense to promote it
##########
superset-frontend/src/features/alerts/components/RecipientIcon.tsx:
##########
@@ -22,7 +22,7 @@ import { Tooltip } from 'src/components/Tooltip';
import Icons from 'src/components/Icons';
import { NotificationMethodOption } from '../types';
-const StyledIcon = (theme: SupersetTheme) => css`
+const NotificationStyledIcon = (theme: SupersetTheme) => css`
Review Comment:
Changing case here as this is not a component but just css. Please, change
all the references down below
##########
superset-frontend/src/features/datasets/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -334,7 +334,10 @@ const DatasetPanel = ({
title={tableName || ''}
>
{tableName && (
- <Icons.Table iconColor={theme.colors.grayscale.base} />
+ <Icons.InsertRowAboveOutlined
+ css={{ margin: '0px!important', verticalAlign: 'baseline' }}
Review Comment:
Let's use the css string and let's remove` !important` if possible.
##########
superset-frontend/src/components/Datasource/CollectionTable.tsx:
##########
@@ -152,6 +152,9 @@ const StyledButtonWrapper = styled.span`
${({ theme }) => `
margin-top: ${theme.gridUnit * 3}px;
margin-left: ${theme.gridUnit * 3}px;
+ button>span>:first-of-type {
+ margin-right:0;
Review Comment:
```suggestion
margin-right: 0;
```
##########
superset-frontend/src/components/Datasource/DatasourceEditor.jsx:
##########
@@ -971,9 +974,21 @@ class DatasourceEditor extends PureComponent {
<EditLockContainer>
<span role="button" tabIndex={0} onClick={this.onChangeEditMode}>
{this.state.isEditMode ? (
- <Icons.LockUnlocked iconColor={theme.colors.grayscale.base} />
+ <Icons.UnlockOutlined
+ iconSize="xl"
+ iconColor={theme.colors.grayscale.base}
+ css={theme => ({
+ margin: `auto ${theme.gridUnit}px auto 0`,
+ })}
+ />
) : (
- <Icons.LockLocked iconColor={theme.colors.grayscale.base} />
+ <Icons.LockOutlined
+ iconSize="xl"
+ iconColor={theme.colors.grayscale.base}
+ css={theme => ({
+ margin: `auto ${theme.gridUnit}px auto 0`,
+ })}
Review Comment:
```suggestion
css={(theme) => css`
margin: auto ${theme.gridUnit}px auto 0;
`}
```
##########
superset-frontend/src/components/MessageToasts/Toast.tsx:
##########
@@ -24,18 +24,26 @@ import Icons from 'src/components/Icons';
import { ToastType, ToastMeta } from './types';
const ToastContainer = styled.div`
- display: flex;
- justify-content: center;
- align-items: center;
+ ${({ theme }) => css`
+ display: flex;
+ justify-content: center;
+ align-items: center;
- span {
- padding: 0 11px;
- }
+ span {
+ padding: 0 ${theme.gridUnit * 2}px;
+ }
+
+ .toast__close,
+ .toast__close span {
+ padding: 0;
+ }
+ `}
`;
-const StyledIcon = (theme: SupersetTheme) => css`
+const NotificationStyledIcon = (theme: SupersetTheme) => css`
Review Comment:
```suggestion
const notificationIconCss = (theme: SupersetTheme) => css`
```
##########
superset-frontend/src/components/DropdownSelectableIcon/DropdownSelectableIcon.stories.tsx:
##########
@@ -0,0 +1,65 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import Icons from 'src/components/Icons';
+import { useTheme } from '@superset-ui/core';
+import DropdownSelectableIcon, { DropDownSelectableProps } from '.';
+
+export default {
+ title: 'DropdownSelectableIcon',
Review Comment:
Thanks for adding this!
##########
superset-frontend/src/components/Datasource/DatasourceEditor.jsx:
##########
@@ -971,9 +974,21 @@ class DatasourceEditor extends PureComponent {
<EditLockContainer>
<span role="button" tabIndex={0} onClick={this.onChangeEditMode}>
{this.state.isEditMode ? (
- <Icons.LockUnlocked iconColor={theme.colors.grayscale.base} />
+ <Icons.UnlockOutlined
+ iconSize="xl"
+ iconColor={theme.colors.grayscale.base}
+ css={theme => ({
+ margin: `auto ${theme.gridUnit}px auto 0`,
+ })}
Review Comment:
```suggestion
css={(theme) => css`
margin: auto ${theme.gridUnit}px auto 0;
`}
```
Can we do this everywhere for consistency with the rest of the codebase?
Just import `css` in the file.
##########
superset-frontend/src/features/alerts/components/RecipientIcon.tsx:
##########
@@ -22,7 +22,7 @@ import { Tooltip } from 'src/components/Tooltip';
import Icons from 'src/components/Icons';
import { NotificationMethodOption } from '../types';
-const StyledIcon = (theme: SupersetTheme) => css`
+const NotificationStyledIcon = (theme: SupersetTheme) => css`
Review Comment:
```suggestion
const notificationIconCss = (theme: SupersetTheme) => css`
```
##########
superset-frontend/src/dashboard/components/SliceAdder.tsx:
##########
@@ -366,7 +366,12 @@ class SliceAdder extends Component<SliceAdderProps,
SliceAdderState> {
)
}
>
- <Icons.PlusSmall />
+ <Icons.PlusOutlined
+ css={theme => ({
+ margin: `auto -${theme.gridUnit * 2}px auto 0 !important`,
Review Comment:
Same as my other comment. Also, can we remove `!important`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
