feat: redesign labels #31575
feat: redesign labels #31575
Conversation
|
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment
|
|
/testenv up |
|
@mistercrunch Processing your ephemeral environment request here. |
|
@mistercrunch Ephemeral environment spinning up at http://54.244.170.62:8080. Credentials are |
|
@kasiazjc you might be interested in this |
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Fix Detected |
|---|---|---|
 |
Non-functional hover state ▹ view | |
|
Invalid Color Format in Text Label Styling ▹ view | |
|
Brittle icon fix using CSS hack ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/src/components/Timer/index.tsx | ✅ |
| superset-frontend/src/components/CachedLabel/index.tsx | ✅ |
| superset-frontend/src/components/Icons/Icon.tsx | ✅ |
| superset-frontend/src/components/Label/reusable/PublishedLabel.tsx | ✅ |
| superset-frontend/src/components/Label/reusable/DatasetTypeLabel.tsx | ✅ |
| superset-frontend/packages/superset-ui-core/src/style/index.tsx | ✅ |
| superset-frontend/src/dashboard/components/PublishedStatus/index.tsx | ✅ |
| superset-frontend/src/components/Label/Label.stories.tsx | ✅ |
| superset-frontend/packages/superset-ui-demo/storybook/stories/superset-ui-style/Theme.stories.tsx | ✅ |
| superset-frontend/src/components/Label/index.tsx | ✅ |
| superset-frontend/src/components/AlteredSliceTag/index.tsx | ✅ |
| superset-frontend/src/pages/DashboardList/index.tsx | ✅ |
| superset-frontend/src/pages/DatasetList/index.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Current Korbit Configuration
General Settings
Setting Value Review Schedule Automatic excluding drafts Max Issue Count 10 Automatic PR Descriptions ❌ Issue Categories
Category Enabled Naming ✅ Database Operations ✅ Documentation ✅ Logging ✅ Error Handling ✅ Systems and Environment ✅ Objects and Data Structures ✅ Readability and Maintainability ✅ Asynchronous Processing ✅ Design Patterns ✅ Third-Party Libraries ✅ Performance ✅ Security ✅ Functionality ✅ Feedback and Support
Note
Korbit Pro is free for open source projects 🎉
Looking to add Korbit to your team? Get started with a free 2 week trial here
| const [hovered] = useState(false); | ||
|
|
||
| const labelType = hovered ? 'primary' : 'default'; |
There was a problem hiding this comment.
Non-functional hover state 
Tell me more
What is the issue?
The hovered state is maintained but the mouse events (onMouseOver and onMouseOut) that would update it have been removed, making the hover effect non-functional.
Why this matters
The label will always remain in the 'default' state since the hovered state can never change to true, breaking the intended hover interaction functionality.
Suggested change
Either remove the unused hover state logic completely:
const labelType = 'default';
Or restore the mouse event handlers:
const [hovered, setHovered] = useState(false);
// ... in Label component:
onMouseOver={() => setHovered(true)}
onMouseOut={() => setHovered(false)}
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
| <h3> | ||
| text.label: <code>{colors.text.label}</code> | ||
| </h3> | ||
| <div style={{ color: `#${colors.text.label}` }}> |
There was a problem hiding this comment.
Invalid Color Format in Text Label Styling
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.
| @@ -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; | |||
There was a problem hiding this comment.
Brittle icon fix using CSS hack
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.
There was a problem hiding this comment.
@rusackas curious if this hack will solve your alignment issue on that other PR (?)
|
@mistercrunch please let me know, if you can assign this issue to me? |
|
@mohdslmn this PR is ready for review/merge, not sure what you mean about assigning it to you (?) |
|
ok got it @mistercrunch |
|
@kasiazjc looks like this may be ready to go if you want to take a look |
| @@ -34,7 +35,7 @@ const CacheLabel: FC<CacheLabelProps> = ({ | |||
| onClick, | |||
| cachedTimestamp, | |||
| }) => { | |||
| const [hovered, setHovered] = useState(false); | |||
| const [hovered] = useState(false); | |||
There was a problem hiding this comment.
Looks like this state variable is redundant now?
| light1: '#B2B2B2', | ||
| light2: '#F0F0F0', | ||
| light3: '#F7F7F7', | ||
| light4: '#FFFFFF', |
There was a problem hiding this comment.
Is it intentional that light4 and light5 are now equal? Same for primary and secondary
There was a problem hiding this comment.
was trying to align the brigthness levels across named colors, bailing on this and adding logic in the theme to point to the right share....
| @@ -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, | |||
There was a problem hiding this comment.
Can we use a named constant for that +4?
Love the change! One thing I would suggest is to keep green for published/checked/done etc (success, error and warning basically), and use some other color (secondary?) for virtual datasets @mistercrunch |
|
/testenv up |
| @@ -25,6 +25,8 @@ import { | |||
|
|
|||
| import { Tag } from 'src/components'; | |||
There was a problem hiding this comment.
It looks like we are pulling Tag directly from Ant Design instead of the recommended approach which is to pull it from a wrapped component (which we have, not sure how it fits with this though). My concern is that when we migrate Tag to Ant Design version 5 some of these changes might need to be reworked. Could this be a good time to upgrade Tag to Ant Design 5 and pull it from the wrapper component everywhere?
There was a problem hiding this comment.
wait I thought that line meant we're pulling from src/components as opposed to antd-v5, which I thought is the prescribed way. Maybe you mean that src/components/Tag should it self wrapped Tag in a styled thing? I think the more vanilla the better.
Maybe the hard rule should be "no-direct-antd-imports!" and maybe even enforce with eslint eventually. But having src/component just be a redirect is fine by me.
There was a problem hiding this comment.
If you look in src/components/index.ts you will see that Tag is being exported directly from antd, which is the older version of Ant Design. What I am proposing here is to use what we have in src/components/Tags/Tag which is already upgraded to Ant Design 5. Not sure if any modifications are necessary to fit that in your code, just saying that we should not be pulling from older antd version.
|
@kasiazjc Ephemeral environment spinning up at http://34.209.140.91:8080. Credentials are |
Realign labels to look and feel closer to native AntD while integrating the theme colors. As part of this PR: - modify <Label> to look and feel like native AntD - adjust theme colors for primary/secondary/grayscale to align brightness-wise, mostly so that `.light2` lines up across the board - Created a reusable label for DatasetTypeLabel and use it it CRUD list view - Creating a PublishedLabel to use for dashboard in header + CRUD - Improvements to storybooks: theme color to line up side by side - Improvement to Label storybook, more interactive
| color = grayscale.light4; | ||
| // TODO - REMOVE IF BLOCK LOGIC WHEN shades are fixed to be aligned in terms of brightness | ||
| // currently shades for >=light2 are not aligned for primary, default and secondary | ||
| if (['default', 'primary', 'secondary'].includes(type)) { |
There was a problem hiding this comment.
@kgabryje I resorted to add custom logic here instead of trying to align the theme. The larger theming effort should fix all of this and remove the need from having ANY color-related logic in the component itself.
latest storybook screenshotshere this first one matrixifying the color grid really shows some of the issues with it, like different colors have different number of shades, and brightness is not aligned across. My other theming PR should really help with that. 
labels
|
| @@ -25,23 +25,96 @@ export default { | |||
|
|
|||
| export const ThemeColors = () => { | |||
| const { colors } = supersetTheme; | |||
| return Object.keys(colors).map(collection => ( | |||
|
|
|||
There was a problem hiding this comment.
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.
|
/testenv up |
|
Looks like |
|
/testenv up |
|
@mistercrunch Processing your ephemeral environment request here. |
|
@mistercrunch Ephemeral environment spinning up at http://52.27.147.237:8080. Credentials are |
|
@mistercrunch yay, was able to test in the test env! 🎉 Two things from me:
Screen.Recording.2025-01-09.at.2.08.10.PM.mov |
|
|
@kasiazjc thanks for the input, moved to using primary/secondary and fixed the hovered state on non-clickable |
Realign labels to look and feel closer to native AntD while integrating the theme colors.
As part of this PR:
.light2lines up across the boardlabel storybook
dashboard header
crud view datasets
crud view dashboards
explore
Description by Korbit AI
What change is being made?
Redesign the label components to enhance UI elements across the application by introducing new styling, reworked storybook stories, and reusable label components.
Why are these changes being made?
The changes aim to improve the clarity, functionality, and visual consistency of labels throughout the app. New components such as
DatasetTypeLabelandPublishedLabelwere introduced to encapsulate reusable label styles and logic, reducing redundancy and potential styling errors. Enhancements in the Theme storybook improve the visualization of theme color palettes, enabling better theming and design coherence.