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

feat: redesign labels #31575

Merged
merged 14 commits into from
Jan 10, 2025
Merged

feat: redesign labels #31575

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
5ae5475
feat: redesign labels
mistercrunch Dec 19, 2024
9d7dd6a
linting
mistercrunch Dec 20, 2024
269f120
wrap in div to enable tooltip
mistercrunch Dec 20, 2024
4b811d5
licens
mistercrunch Dec 20, 2024
6a46815
gpt
mistercrunch Dec 20, 2024
3755f16
recovering state
mistercrunch Jan 7, 2025
1223ca5
try to align
mistercrunch Jan 7, 2025
913c25d
going with conditions in component
mistercrunch Jan 7, 2025
324e636
cleaning up
mistercrunch Jan 7, 2025
4f26653
fixups
mistercrunch Jan 7, 2025
db6b8b5
fixing lint
mistercrunch Jan 7, 2025
e960903
fix ts type
mistercrunch Jan 7, 2025
05bdba2
addressing design-related comments
mistercrunch Jan 9, 2025
f53f183
fix type error
mistercrunch Jan 9, 2025
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
105 changes: 89 additions & 16 deletions ...-frontend/packages/superset-ui-demo/storybook/stories/superset-ui-style/Theme.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
*
* 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
* "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.
*/
Expand All @@ -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>
));
);
};
21 changes: 7 additions & 14 deletions superset-frontend/src/components/AlteredSliceTag/index.tsx
View file
Open in desktop
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 @@ -225,10 +215,13 @@ const AlteredSliceTag: FC<AlteredSliceTagProps> = props => {
);
}, [rows]);

const icon = <Icons.Warning iconSize="m" />;
const triggerNode = useMemo(
() => (
<Tooltip id="difference-tooltip" title={t('Click to see difference')}>
<StyledLabel className="label">{t('Altered')}</StyledLabel>
<Label icon={icon} className="label" type="alert" onClick={() => {}}>
{t('Altered')}
</Label>
</Tooltip>
),
[],
Expand Down
4 changes: 4 additions & 0 deletions superset-frontend/src/components/Icons/Icon.tsx
View file
Open in desktop
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 {
// Hack to fix issues with some icons
line-height: 0px !important;
Copy link

Choose a reason for hiding this comment

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

Brittle icon fix using CSS hack category Functionality

Tell me more
What is the issue?

Using !important and a hack to fix icon issues could lead to unintended side effects with different icon sets or future updates.

Why this matters

This brittle solution might break when new icons are added or when the underlying icon library is updated, potentially causing visual inconsistencies.

Suggested change

Investigate the root cause of the icon alignment issues and implement a more robust solution, possibly by properly configuring the icon component's baseline alignment or using proper spacing properties.

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@rusackas curious if this hack will solve your alignment issue on that other PR (?)

}
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
View file
Open in desktop
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' },
},
};
2 changes: 1 addition & 1 deletion superset-frontend/src/components/Label/Label.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,6 @@ test('works with an onClick handler', () => {
test('renders all the storybook gallery variants', () => {
const { container } = render(<LabelGallery />);
expect(container.querySelectorAll('.ant-tag')).toHaveLength(
options.length * 2,
options.length * 2 + 4,
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 use a named constant for that +4?

);
});
Loading
Loading