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.

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

feat: redesign labels #31575

Merged
merged 14 commits into from
Jan 10, 2025
Original file line number Diff line number Diff line change
Expand Up @@ -25,23 +25,96 @@ export default {

export const ThemeColors = () => {
const { colors } = supersetTheme;
return Object.keys(colors).map(collection => (

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is improving the Theme Colors storybook as I kind of needed that to see throught his PR. Not directly related to the PR but helpful overall.

// Define tones to be displayed in columns
const tones = [
'dark2',
'dark1',
'base',
'light1',
'light2',
'light3',
'light4',
'light5',
];
const colorTypes = [
'primary',
'secondary',
'grayscale',
'error',
'warning',
'alert',
'success',
'info',
];
return (
<div>
<h2>{collection}</h2>
<table style={{ width: '300px' }}>
{Object.keys(colors[collection]).map(k => {
const hex = colors[collection][k];
return (
<tr>
<td>{k}</td>
<td>
<code>{hex}</code>
<h1>Theme Colors</h1>
<table
style={{ borderCollapse: 'collapse', width: '100%', textAlign: 'left' }}
>
<thead>
<tr>
<th style={{ border: '1px solid #ddd', padding: '8px' }}>
Category
</th>
{tones.map(tone => (
<th
key={tone}
style={{ border: '1px solid #ddd', padding: '8px' }}
>
{tone}
</th>
))}
</tr>
</thead>
<tbody>
{colorTypes.map(category => (
<tr key={category}>
<td style={{ border: '1px solid #ddd', padding: '8px' }}>
<strong>{category}</strong>
</td>
<td style={{ width: '150px', backgroundColor: hex }} />
{tones.map(tone => {
const color = colors[category][tone];
return (
<td
key={tone}
style={{
border: '1px solid #ddd',
padding: '8px',
backgroundColor: color || '#fff',
}}
>
{color ? <code>{color}</code> : '-'}
</td>
);
})}
</tr>
);
})}
))}
</tbody>
</table>
<h3>
text.label: <code>{colors.text.label}</code>
</h3>
<div style={{ color: `#${colors.text.label}` }}>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invalid Color Format in Text Label Styling category Functionality

Tell me more
What is the issue?

The code incorrectly prepends '#' to the color value from colors.text.label, which likely already includes the '#' prefix, resulting in an invalid color format.

Why this matters

This will cause the text to be rendered with an invalid color value, making it invisible or using the browser's default color instead of the intended theme color.

Suggested change

Remove the '#' prefix: style={{ color: colors.text.label }}

Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed do eiusmod
tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim
veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea
commodo consequat.
</div>
<h3>
text.help: <code>{colors.text.help}</code>
</h3>
<div style={{ color: `#${colors.text.help}` }}>
Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed do eiusmod
tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim
veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea
commodo consequat.
</div>
<h3>The supersetTheme object</h3>
<code>
<pre>{JSON.stringify(supersetTheme, null, 2)}</pre>
</code>
</div>
));
);
};
25 changes: 11 additions & 14 deletions superset-frontend/src/components/AlteredSliceTag/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,11 @@
import { useCallback, useEffect, useMemo, useState, FC } from 'react';

import { isEqual, isEmpty } from 'lodash';
import { QueryFormData, styled, t } from '@superset-ui/core';
import { QueryFormData, t } from '@superset-ui/core';
import { sanitizeFormData } from 'src/explore/exploreUtils/formData';
import getControlsForVizType from 'src/utils/getControlsForVizType';
import Label from 'src/components/Label';
import Icons from 'src/components/Icons';
import { safeStringify } from 'src/utils/safeStringify';
import { Tooltip } from 'src/components/Tooltip';
import ModalTrigger from '../ModalTrigger';
Expand Down Expand Up @@ -68,18 +70,6 @@ export type RowType = {
control: string;
};

const StyledLabel = styled.span`
${({ theme }) => `
font-size: ${theme.typography.sizes.s}px;
color: ${theme.colors.grayscale.dark1};
background-color: ${theme.colors.alert.base};
&:hover {
background-color: ${theme.colors.alert.dark1};
}
`}
`;

export const alterForComparison = (
value?: string | null | [],
): string | null => {
Expand Down Expand Up @@ -228,7 +218,14 @@ const AlteredSliceTag: FC<AlteredSliceTagProps> = props => {
const triggerNode = useMemo(
() => (
<Tooltip id="difference-tooltip" title={t('Click to see difference')}>
<StyledLabel className="label">{t('Altered')}</StyledLabel>
<Label
icon={<Icons.Warning iconSize="m" />}
className="label"
type="alert"
onClick={() => {}}
>
{t('Altered')}
</Label>
</Tooltip>
),
[],
Expand Down
4 changes: 4 additions & 0 deletions superset-frontend/src/components/Icons/Icon.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ const AntdIconComponent = ({

export const StyledIcon = styled(AntdIconComponent)<IconType>`
${({ iconColor }) => iconColor && `color: ${iconColor};`};
span {
// Fixing alignement on some of the icons
line-height: 0px;
}
font-size: ${({ iconSize, theme }) =>
iconSize
? `${theme.typography.sizes[iconSize] || theme.typography.sizes.m}px`
Expand Down
88 changes: 56 additions & 32 deletions superset-frontend/src/components/Label/Label.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,17 @@
* under the License.
*/
import { action } from '@storybook/addon-actions';
import Label, { Type } from './index';
import { Meta, StoryFn } from '@storybook/react';
import Label, { Type, DatasetTypeLabel, PublishedLabel } from './index';

// Define the default export with Storybook configuration
export default {
title: 'Label',
component: Label,
excludeStories: 'options',
};
excludeStories: ['options'],
} as Meta<typeof Label>;

// Explicitly type the options array as an array of `Type`
export const options: Type[] = [
'default',
'alert',
Expand All @@ -36,39 +39,60 @@ export const options: Type[] = [
'secondary',
];

export const LabelGallery = () => (
<>
<h4>Non-interactive</h4>
{Object.values(options).map((opt: Type) => (
<Label key={opt} type={opt}>
{`style: "${opt}"`}
</Label>
))}
<br />
<h4>Interactive</h4>
{Object.values(options).map((opt: Type) => (
<Label key={opt} type={opt} onClick={action('clicked')}>
{`style: "${opt}"`}
</Label>
))}
</>
);
// Define the props for the `LabelGallery` component
interface LabelGalleryProps {
hasOnClick?: boolean;
monospace?: boolean;
}

// Use the `StoryFn` type for LabelGallery
export const LabelGallery: StoryFn<LabelGalleryProps> = (
props: LabelGalleryProps,
) => {
const onClick = props.hasOnClick ? action('clicked') : undefined;

export const InteractiveLabel = (args: any) => {
const { hasOnClick, label, monospace, ...rest } = args;
return (
<Label
onClick={hasOnClick ? action('clicked') : undefined}
monospace={monospace}
{...rest}
>
{label}
</Label>
<>
<h4>Non-interactive</h4>
{options.map((opt: Type) => (
<Label key={opt} type={opt}>
{`style: "${opt}"`}
</Label>
))}
<br />
<h4>Interactive</h4>
{options.map((opt: Type) => (
<Label key={opt} type={opt} {...props} onClick={onClick}>
{`style: "${opt}"`}
</Label>
))}
<h4>Reusable Labels</h4>
<h5>DatasetType</h5>
<div>
<DatasetTypeLabel datasetType="physical" />
<DatasetTypeLabel datasetType="virtual" />
</div>
<h5>PublishedLabel</h5>
<PublishedLabel isPublished />
<PublishedLabel isPublished={false} />
</>
);
};

InteractiveLabel.args = {
// Define default arguments for Storybook
LabelGallery.args = {
hasOnClick: true,
label: 'Example',
monospace: true,
monospace: false,
};

// Define argument types for Storybook controls
LabelGallery.argTypes = {
monospace: {
name: 'monospace',
control: { type: 'boolean' },
},
hasOnClick: {
name: 'hasOnClick',
control: { type: 'boolean' },
},
};
4 changes: 3 additions & 1 deletion superset-frontend/src/components/Label/Label.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,9 @@ test('works with an onClick handler', () => {
// test stories from the storybook!
test('renders all the storybook gallery variants', () => {
const { container } = render(<LabelGallery />);
const nonInteractiveLabelCount = 4;
const renderedLabelCount = options.length * 2 + nonInteractiveLabelCount;
expect(container.querySelectorAll('.ant-tag')).toHaveLength(
options.length * 2,
renderedLabelCount,
);
});
Loading
Loading