geido commented on code in PR #32959:
URL: https://github.com/apache/superset/pull/32959#discussion_r2024616539
##########
superset-frontend/src/SqlLab/components/TableElement/index.tsx:
##########
@@ -273,77 +242,90 @@ 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')}
+ className={
+ `fa fa-sort-${sortColumns ? 'numeric' : 'alpha'}-asc ` +
Review Comment:
We are removing usages of fa icons from the codebase. Can you use the Icons
component from `src/components/Icons `instead?
##########
superset-frontend/src/SqlLab/components/TableElement/index.tsx:
##########
@@ -273,77 +242,90 @@ 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')}
+ 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={
+ <IconTooltip
+ aria-label="Copy"
Review Comment:
Let's localize this too. I see inconsistent localization with this prop
across the codebase but I guess localizing won't hurt.
##########
superset-frontend/src/explore/components/controls/FixedOrMetricControl/index.jsx:
##########
@@ -122,67 +123,70 @@ export default class FixedOrMetricControl extends
Component {
}
}
`}
- >
- <Collapse.Panel
- showArrow={false}
- header={
- <Label>
- {this.state.type === controlTypes.fixed && (
- <span>{this.state.fixedValue}</span>
- )}
- {this.state.type === controlTypes.metric && (
- <span>
- <span>{t('metric')}: </span>
- <strong>
- {this.state.metricValue
- ? this.state.metricValue.label
- : null}
- </strong>
- </span>
- )}
- </Label>
- }
- >
- <div className="well">
- <PopoverSection
- title={t('Fixed')}
- isSelected={type === controlTypes.fixed}
- onSelect={() => {
- this.setType(controlTypes.fixed);
- }}
- >
- <TextControl
- isFloat
- onChange={this.setFixedValue}
- onFocus={() => {
- this.setType(controlTypes.fixed);
- }}
- value={this.state.fixedValue}
- />
- </PopoverSection>
- <PopoverSection
- title={t('Based on a metric')}
- isSelected={type === controlTypes.metric}
- onSelect={() => {
- this.setType(controlTypes.metric);
- }}
- >
- <MetricsControl
- name="metric"
- columns={columns}
- savedMetrics={metrics}
- multi={false}
- onFocus={() => {
- this.setType(controlTypes.metric);
- }}
- onChange={this.setMetric}
- value={this.state.metricValue}
- datasource={this.props.datasource}
- />
- </PopoverSection>
- </div>
- </Collapse.Panel>
- </Collapse>
+ items={[
+ {
+ key: '1',
Review Comment:
Can we use human-readable keys?
##########
superset-frontend/src/pages/Home/index.tsx:
##########
@@ -364,55 +374,65 @@ function Welcome({ user, addDangerToast }: WelcomeProps) {
onChange={handleCollapse}
ghost
bigger
- >
- <Collapse.Panel header={t('Recents')} key="1">
- {activityData &&
- (activityData[TableTab.Viewed] ||
- activityData[TableTab.Other] ||
- activityData[TableTab.Created]) &&
- activeChild !== 'Loading' ? (
- <ActivityTable
- user={{ userId: user.userId! }} // user is definitely not
a guest user on this page
- activeChild={activeChild}
- setActiveChild={setActiveChild}
- activityData={activityData}
- isFetchingActivityData={isFetchingActivityData}
- />
- ) : (
- <LoadingCards />
- )}
- </Collapse.Panel>
- <Collapse.Panel header={t('Dashboards')} key="2">
- {!dashboardData || isRecentActivityLoading ? (
- <LoadingCards cover={checked} />
- ) : (
- <DashboardTable
- user={user}
- mine={dashboardData}
- showThumbnails={checked}
- otherTabData={activityData?.[TableTab.Other]}
- otherTabFilters={otherTabFilters}
- otherTabTitle={otherTabTitle}
- />
- )}
- </Collapse.Panel>
- <Collapse.Panel header={t('Charts')} key="3">
- {!chartData || isRecentActivityLoading ? (
- <LoadingCards cover={checked} />
- ) : (
- <ChartTable
- showThumbnails={checked}
- user={user}
- mine={chartData}
- otherTabData={activityData?.[TableTab.Other]}
- otherTabFilters={otherTabFilters}
- otherTabTitle={otherTabTitle}
- />
- )}
- </Collapse.Panel>
- {canReadSavedQueries && (
- <Collapse.Panel header={t('Saved queries')} key="4">
- {!queryData ? (
+ items={[
+ {
+ key: '1',
Review Comment:
same comment about human-readable
##########
superset-frontend/src/components/Collapse/index.tsx:
##########
@@ -29,74 +29,72 @@ export interface CollapseProps extends AntdCollapseProps {
animateArrows?: boolean;
}
-const Collapse = Object.assign(
- // eslint-disable-next-line @typescript-eslint/no-unused-vars
- styled(({ light, bigger, bold, animateArrows, ...props }: CollapseProps) => (
- <AntdCollapse {...props} />
- ))`
- .ant-collapse-item {
- .ant-collapse-header {
- font-weight: ${({ bold, theme }) =>
- bold ? theme.fontWeightStrong : theme.fontWeightNormal};
- font-size: ${({ bigger, theme }) =>
- bigger ? `${theme.sizeUnit * 4}px` : 'inherit'};
+const Collapse = styled((props: CollapseProps) => <AntdCollapse {...props} />)`
+ .antd5-collapse-item {
+ .antd5-collapse-header {
+ font-weight: ${({ bold, theme }) =>
+ bold ? theme.fontWeightStrong : theme.fontWeightNormal};
+ font-size: ${({ bigger, theme }) =>
+ bigger ? `${theme.sizeUnit * 4}px` : 'inherit'};
- .ant-collapse-arrow svg {
- transition: ${({ animateArrows }) =>
- animateArrows ? 'transform 0.24s' : 'none'};
- }
+ .antd5-collapse-arrow svg {
+ transition: ${({ animateArrows }) =>
+ animateArrows ? 'transform 0.24s' : 'none'};
+ }
- ${({ expandIconPosition }) =>
- expandIconPosition &&
- expandIconPosition === 'right' &&
- `
- .anticon.anticon-right.ant-collapse-arrow > svg {
+ ${({ expandIconPosition }) =>
Review Comment:
This is all necessary because Ant Design does not support arrows on the
right?
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/OAuth2ClientField.tsx:
##########
@@ -91,51 +79,55 @@ export const OAuth2ClientField = ({
};
return (
- <Collapse>
- <Collapse.Panel
- header="OAuth2 client information"
- key="1"
- css={collapseStyle}
- >
- <FormItem label={LABELS.CLIENT_ID}>
- <Input
- data-test="client-id"
- value={oauth2ClientInfo.id}
- onChange={handleChange('id')}
- />
- </FormItem>
- <FormItem label={LABELS.SECRET}>
- <Input
- data-test="client-secret"
- type="password"
- value={oauth2ClientInfo.secret}
- onChange={handleChange('secret')}
- />
- </FormItem>
- <FormItem label={LABELS.AUTH_URI}>
- <Input
- data-test="client-authorization-request-uri"
- placeholder="https://";
- value={oauth2ClientInfo.authorization_request_uri}
- onChange={handleChange('authorization_request_uri')}
- />
- </FormItem>
- <FormItem label={LABELS.TOKEN_URI}>
- <Input
- data-test="client-token-request-uri"
- placeholder="https://";
- value={oauth2ClientInfo.token_request_uri}
- onChange={handleChange('token_request_uri')}
- />
- </FormItem>
- <FormItem label={LABELS.SCOPE}>
- <Input
- data-test="client-scope"
- value={oauth2ClientInfo.scope}
- onChange={handleChange('scope')}
- />
- </FormItem>
- </Collapse.Panel>
- </Collapse>
+ <Collapse
+ items={[
+ {
+ key: '1',
Review Comment:
human-readable if possible
--
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]
