msyavuz commented on code in PR #32959:
URL: https://github.com/apache/superset/pull/32959#discussion_r2025511662
##########
superset-frontend/src/components/ErrorMessage/MarshmallowErrorMessage.tsx:
##########
@@ -41,9 +41,11 @@ const collapseStyle = (theme: SupersetTheme) => css`
.ant-collapse-arrow {
left: 0px !important;
}
+
.ant-collapse-header {
Review Comment:
Won't these classnames change to antd5?
##########
superset-frontend/src/SqlLab/components/TableElement/index.tsx:
##########
@@ -273,77 +242,96 @@ const TableElement = ({ table, ...props }:
TableElementProps) => {
);
}
return (
- <ButtonGroup
- css={css`
- column-gap: ${theme.sizeUnit * 1.5}px;
- margin-right: ${theme.sizeUnit}px;
- & span {
- display: flex;
- justify-content: center;
- width: ${theme.sizeUnit * 4}px;
- }
- `}
- >
- <IconTooltip
- className="pull-left m-l-2 pointer"
- onClick={refreshTableMetadata}
- tooltip={t('Refresh table schema')}
- >
- <Icons.SyncOutlined
- iconSize="m"
- iconColor={theme.colors.primary.dark2}
- />
- </IconTooltip>
- {keyLink}
- <IconTooltip
- className={
- `fa fa-sort-${sortColumns ? 'numeric' : 'alpha'}-asc ` +
- 'pull-left sort-cols m-l-2 pointer'
- }
- onClick={toggleSortColumns}
- tooltip={
- sortColumns
- ? t('Original table column order')
- : t('Sort columns alphabetically')
- }
- />
- {tableData.selectStar && (
- <CopyToClipboard
- copyNode={
+ <StyledControls>
+ {isMetadataFetching || isExtraMetadataLoading ? (
+ <Loading position="inline" />
+ ) : (
+ <Fade
+ data-test="fade"
+ hovered={hovered}
+ onClick={e => e.stopPropagation()}
+ >
+ <ButtonGroup
+ css={css`
+ column-gap: ${theme.sizeUnit * 1.5}px;
+ margin-right: ${theme.sizeUnit}px;
+
+ & span {
+ display: flex;
+ justify-content: center;
+ width: ${theme.sizeUnit * 4}px;
+ }
+ `}
+ >
+ <IconTooltip
+ className="pull-left m-l-2 pointer"
+ onClick={refreshTableMetadata}
+ tooltip={t('Refresh table schema')}
+ >
+ <Icons.SyncOutlined
+ iconSize="m"
+ iconColor={theme.colors.grayscale.dark5}
+ />
+ </IconTooltip>
+ {keyLink}
<IconTooltip
- aria-label="Copy"
- tooltip={t('Copy SELECT statement to the clipboard')}
+ onClick={toggleSortColumns}
+ tooltip={
+ sortColumns
+ ? t('Original table column order')
+ : t('Sort columns alphabetically')
+ }
>
- <Icons.CopyOutlined
+ <Icons.SortAscendingOutlined
iconSize="m"
- iconColor={theme.colors.primary.dark2}
aria-hidden
+ iconColor={
+ sortColumns
+ ? theme.colors.grayscale.dark5
Review Comment:
Same here
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -909,397 +884,444 @@ const FiltersConfigForm = (
)}
</StyledRowContainer>
)}
- <StyledCollapse
+ <Collapse
defaultActiveKey={activeFilterPanelKeys}
onChange={key => {
handleActiveFilterPanelChange(key);
}}
expandIconPosition="right"
key={`native-filter-config-${filterId}`}
- >
- {formFilter?.filterType !== 'filter_time' && (
- <Collapse.Panel
- forceRender
- header={FilterPanels.configuration.name}
- key={`${filterId}-${FilterPanels.configuration.key}`}
- >
- {canDependOnOtherFilters && hasAvailableFilters && (
- <StyledRowFormItem
- expanded={expanded}
- name={['filters', filterId, 'dependencies']}
- initialValue={dependencies}
- >
- <DependencyList
- availableFilters={availableFilters}
- dependencies={dependencies}
- onDependenciesChange={dependencies => {
- setNativeFilterFieldValues(form, filterId, {
- dependencies,
- });
- forceUpdate();
- validateDependencies();
- formChanged();
- }}
- getDependencySuggestion={() =>
- getDependencySuggestion(filterId)
- }
- >
- {hasTimeDependency ? timeColumn : undefined}
- </DependencyList>
- </StyledRowFormItem>
- )}
- {hasDataset && hasAdditionalFilters && (
- <FormItem name={['filters', filterId, 'preFilter']}>
- <CollapsibleControl
- initialValue={hasPreFilter}
- title={t('Pre-filter available values')}
- tooltip={t(`Add filter clauses to control the filter's
source query,
+ items={[
+ ...(formFilter?.filterType !== 'filter_time'
+ ? [
+ {
+ key: `${filterId}-${FilterPanels.configuration.key}`,
+ label: FilterPanels.configuration.name,
+ children: (
Review Comment:
Is losing forceRender not important here?
##########
superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx:
##########
@@ -276,20 +231,16 @@ const SqlEditorLeftBar = ({
height: ${tableMetaDataHeight}px;
`}
>
- <Collapse
- activeKey={tables
- .filter(({ expanded }) => expanded)
- .map(({ id }) => id)}
- css={collapseStyles}
- expandIconPosition="right"
- ghost
- onChange={onToggleTable}
- expandIcon={renderExpandIconWithTooltip}
- >
- {tables.map(table => (
- <TableElement table={table} key={table.id} />
- ))}
- </Collapse>
+ {tables.map(table => (
+ <TableElement
+ table={table}
+ key={table.id}
+ activeKey={tables
+ .filter(({ expanded }) => expanded)
+ .map(({ id }) => id)}
+ onChange={onToggleTable}
+ />
+ ))}
Review Comment:
I am not sure why this refactor of Collapse into TableElement necessary?
##########
superset-frontend/src/SqlLab/components/TableElement/index.tsx:
##########
@@ -273,77 +242,96 @@ const TableElement = ({ table, ...props }:
TableElementProps) => {
);
}
return (
- <ButtonGroup
- css={css`
- column-gap: ${theme.sizeUnit * 1.5}px;
- margin-right: ${theme.sizeUnit}px;
- & span {
- display: flex;
- justify-content: center;
- width: ${theme.sizeUnit * 4}px;
- }
- `}
- >
- <IconTooltip
- className="pull-left m-l-2 pointer"
- onClick={refreshTableMetadata}
- tooltip={t('Refresh table schema')}
- >
- <Icons.SyncOutlined
- iconSize="m"
- iconColor={theme.colors.primary.dark2}
- />
- </IconTooltip>
- {keyLink}
- <IconTooltip
- className={
- `fa fa-sort-${sortColumns ? 'numeric' : 'alpha'}-asc ` +
- 'pull-left sort-cols m-l-2 pointer'
- }
- onClick={toggleSortColumns}
- tooltip={
- sortColumns
- ? t('Original table column order')
- : t('Sort columns alphabetically')
- }
- />
- {tableData.selectStar && (
- <CopyToClipboard
- copyNode={
+ <StyledControls>
+ {isMetadataFetching || isExtraMetadataLoading ? (
+ <Loading position="inline" />
+ ) : (
+ <Fade
+ data-test="fade"
+ hovered={hovered}
+ onClick={e => e.stopPropagation()}
+ >
+ <ButtonGroup
+ css={css`
+ column-gap: ${theme.sizeUnit * 1.5}px;
+ margin-right: ${theme.sizeUnit}px;
+
+ & span {
+ display: flex;
+ justify-content: center;
+ width: ${theme.sizeUnit * 4}px;
+ }
+ `}
+ >
+ <IconTooltip
+ className="pull-left m-l-2 pointer"
+ onClick={refreshTableMetadata}
+ tooltip={t('Refresh table schema')}
+ >
+ <Icons.SyncOutlined
+ iconSize="m"
+ iconColor={theme.colors.grayscale.dark5}
+ />
+ </IconTooltip>
+ {keyLink}
<IconTooltip
- aria-label="Copy"
- tooltip={t('Copy SELECT statement to the clipboard')}
+ onClick={toggleSortColumns}
+ tooltip={
+ sortColumns
+ ? t('Original table column order')
+ : t('Sort columns alphabetically')
+ }
>
- <Icons.CopyOutlined
+ <Icons.SortAscendingOutlined
iconSize="m"
- iconColor={theme.colors.primary.dark2}
aria-hidden
+ iconColor={
+ sortColumns
+ ? theme.colors.grayscale.dark5
+ : theme.colorTextDisabled
+ }
/>
</IconTooltip>
- }
- text={tableData.selectStar}
- shouldShowText={false}
- />
- )}
- {tableData.view && (
- <ShowSQL
- sql={tableData.view}
- tooltipText={t('Show CREATE VIEW statement')}
- title={t('CREATE VIEW statement')}
- />
+ {tableData.selectStar && (
+ <CopyToClipboard
+ copyNode={
+ <IconTooltip
+ aria-label={t('Copy')}
+ tooltip={t('Copy SELECT statement to the clipboard')}
+ >
+ <Icons.CopyOutlined
+ iconSize="m"
+ aria-hidden
+ iconColor={theme.colors.grayscale.dark5}
+ />
+ </IconTooltip>
+ }
+ text={tableData.selectStar}
+ shouldShowText={false}
+ />
+ )}
+ {tableData.view && (
+ <ShowSQL
+ sql={tableData.view}
+ tooltipText={t('Show CREATE VIEW statement')}
+ title={t('CREATE VIEW statement')}
+ />
+ )}
+ <IconTooltip
+ className=" table-remove pull-left m-l-2 pointer"
+ onClick={removeTable}
+ tooltip={t('Remove table preview')}
+ >
+ <Icons.CloseOutlined
+ iconSize="m"
+ aria-hidden
+ iconColor={theme.colors.grayscale.dark5}
Review Comment:
and here
##########
superset-frontend/src/SqlLab/components/TableElement/index.tsx:
##########
@@ -273,77 +242,96 @@ const TableElement = ({ table, ...props }:
TableElementProps) => {
);
}
return (
- <ButtonGroup
- css={css`
- column-gap: ${theme.sizeUnit * 1.5}px;
- margin-right: ${theme.sizeUnit}px;
- & span {
- display: flex;
- justify-content: center;
- width: ${theme.sizeUnit * 4}px;
- }
- `}
- >
- <IconTooltip
- className="pull-left m-l-2 pointer"
- onClick={refreshTableMetadata}
- tooltip={t('Refresh table schema')}
- >
- <Icons.SyncOutlined
- iconSize="m"
- iconColor={theme.colors.primary.dark2}
- />
- </IconTooltip>
- {keyLink}
- <IconTooltip
- className={
- `fa fa-sort-${sortColumns ? 'numeric' : 'alpha'}-asc ` +
- 'pull-left sort-cols m-l-2 pointer'
- }
- onClick={toggleSortColumns}
- tooltip={
- sortColumns
- ? t('Original table column order')
- : t('Sort columns alphabetically')
- }
- />
- {tableData.selectStar && (
- <CopyToClipboard
- copyNode={
+ <StyledControls>
+ {isMetadataFetching || isExtraMetadataLoading ? (
+ <Loading position="inline" />
+ ) : (
+ <Fade
+ data-test="fade"
+ hovered={hovered}
+ onClick={e => e.stopPropagation()}
+ >
+ <ButtonGroup
+ css={css`
+ column-gap: ${theme.sizeUnit * 1.5}px;
+ margin-right: ${theme.sizeUnit}px;
+
+ & span {
+ display: flex;
+ justify-content: center;
+ width: ${theme.sizeUnit * 4}px;
+ }
+ `}
+ >
+ <IconTooltip
+ className="pull-left m-l-2 pointer"
+ onClick={refreshTableMetadata}
+ tooltip={t('Refresh table schema')}
+ >
+ <Icons.SyncOutlined
+ iconSize="m"
+ iconColor={theme.colors.grayscale.dark5}
Review Comment:
I think we are trying to use Ant Design tokens whenever we can so i am not
sure but this would roughly map to `theme.colorIcon`?
##########
superset-frontend/src/SqlLab/components/TableElement/index.tsx:
##########
@@ -273,77 +242,96 @@ const TableElement = ({ table, ...props }:
TableElementProps) => {
);
}
return (
- <ButtonGroup
- css={css`
- column-gap: ${theme.sizeUnit * 1.5}px;
- margin-right: ${theme.sizeUnit}px;
- & span {
- display: flex;
- justify-content: center;
- width: ${theme.sizeUnit * 4}px;
- }
- `}
- >
- <IconTooltip
- className="pull-left m-l-2 pointer"
- onClick={refreshTableMetadata}
- tooltip={t('Refresh table schema')}
- >
- <Icons.SyncOutlined
- iconSize="m"
- iconColor={theme.colors.primary.dark2}
- />
- </IconTooltip>
- {keyLink}
- <IconTooltip
- className={
- `fa fa-sort-${sortColumns ? 'numeric' : 'alpha'}-asc ` +
- 'pull-left sort-cols m-l-2 pointer'
- }
- onClick={toggleSortColumns}
- tooltip={
- sortColumns
- ? t('Original table column order')
- : t('Sort columns alphabetically')
- }
- />
- {tableData.selectStar && (
- <CopyToClipboard
- copyNode={
+ <StyledControls>
+ {isMetadataFetching || isExtraMetadataLoading ? (
+ <Loading position="inline" />
+ ) : (
+ <Fade
+ data-test="fade"
+ hovered={hovered}
+ onClick={e => e.stopPropagation()}
+ >
+ <ButtonGroup
+ css={css`
+ column-gap: ${theme.sizeUnit * 1.5}px;
+ margin-right: ${theme.sizeUnit}px;
+
+ & span {
+ display: flex;
+ justify-content: center;
+ width: ${theme.sizeUnit * 4}px;
+ }
+ `}
+ >
+ <IconTooltip
+ className="pull-left m-l-2 pointer"
+ onClick={refreshTableMetadata}
+ tooltip={t('Refresh table schema')}
+ >
+ <Icons.SyncOutlined
+ iconSize="m"
+ iconColor={theme.colors.grayscale.dark5}
+ />
+ </IconTooltip>
+ {keyLink}
<IconTooltip
- aria-label="Copy"
- tooltip={t('Copy SELECT statement to the clipboard')}
+ onClick={toggleSortColumns}
+ tooltip={
+ sortColumns
+ ? t('Original table column order')
+ : t('Sort columns alphabetically')
+ }
>
- <Icons.CopyOutlined
+ <Icons.SortAscendingOutlined
iconSize="m"
- iconColor={theme.colors.primary.dark2}
aria-hidden
+ iconColor={
+ sortColumns
+ ? theme.colors.grayscale.dark5
+ : theme.colorTextDisabled
+ }
/>
</IconTooltip>
- }
- text={tableData.selectStar}
- shouldShowText={false}
- />
- )}
- {tableData.view && (
- <ShowSQL
- sql={tableData.view}
- tooltipText={t('Show CREATE VIEW statement')}
- title={t('CREATE VIEW statement')}
- />
+ {tableData.selectStar && (
+ <CopyToClipboard
+ copyNode={
+ <IconTooltip
+ aria-label={t('Copy')}
+ tooltip={t('Copy SELECT statement to the clipboard')}
+ >
+ <Icons.CopyOutlined
+ iconSize="m"
+ aria-hidden
+ iconColor={theme.colors.grayscale.dark5}
Review Comment:
and here
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
