You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mistercrunch (via GitHub)" <gi...@apache.org> on 2024/12/20 01:03:03 UTC
[PR] feat: redesign labels [superset]
mistercrunch opened a new pull request, #31575:
URL: https://github.com/apache/superset/pull/31575
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
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #31575:
URL: https://github.com/apache/superset/pull/31575#discussion_r1894535611
##########
superset-frontend/packages/superset-ui-demo/storybook/stories/superset-ui-style/Theme.stories.tsx:
##########
@@ -25,23 +25,96 @@ export default {
export const ThemeColors = () => {
const { colors } = supersetTheme;
- return Object.keys(colors).map(collection => (
+
+ // 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}` }}>
Review Comment:
### Invalid Color Format in Text Label Styling <sub></sub>
<details>
<summary>Tell me more</summary>
###### 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 }}
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
<!--- korbi internal id:881a7e23-e449-4398-9f45-c806fb13ea02 -->
##########
superset-frontend/src/components/CachedLabel/index.tsx:
##########
@@ -34,7 +35,7 @@ const CacheLabel: FC<CacheLabelProps> = ({
onClick,
cachedTimestamp,
}) => {
- const [hovered, setHovered] = useState(false);
+ const [hovered] = useState(false);
const labelType = hovered ? 'primary' : 'default';
Review Comment:
### Non-functional hover state <sub></sub>
<details>
<summary>Tell me more</summary>
###### 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)}
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
<!--- korbi internal id:b94a5315-f709-409b-9029-a604f4d33e13 -->
##########
superset-frontend/src/components/Icons/Icon.tsx:
##########
@@ -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;
Review Comment:
### Brittle icon fix using CSS hack <sub></sub>
<details>
<summary>Tell me more</summary>
###### 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.
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
<!--- korbi internal id:e77f722a-a326-4119-8edf-738d8fadd891 -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2581326774
<img width="884" alt="Screenshot 2025-01-09 at 2 02 08 PM" src="https://github.com/user-attachments/assets/14bec8c5-7502-43aa-ad68-b21a5bea200c" />
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31575:
URL: https://github.com/apache/superset/pull/31575#discussion_r1905983050
##########
superset-frontend/src/components/Icons/Icon.tsx:
##########
@@ -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;
Review Comment:
@rusackas curious if this hack will solve your alignment issue on that other PR (?)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2564378758
@mohdslmn this PR is ready for review/merge, not sure what you mean about assigning it to you (?)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31575:
URL: https://github.com/apache/superset/pull/31575#discussion_r1907740212
##########
superset-frontend/src/components/Label/index.tsx:
##########
@@ -25,6 +25,8 @@ import {
import { Tag } from 'src/components';
Review Comment:
wait I thought that line meant we're pulling from `src/components` as opposed to antd-v5, which 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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2578770376
@eschutho Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/12679921092).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31575:
URL: https://github.com/apache/superset/pull/31575#discussion_r1905984515
##########
superset-frontend/src/components/Label/index.tsx:
##########
@@ -71,49 +75,57 @@ export default function Label(props: LabelProps) {
info,
} = colors;
- let backgroundColor = grayscale.light3;
- let backgroundColorHover = onClick ? primary.light2 : grayscale.light3;
- let borderColor = onClick ? grayscale.light2 : 'transparent';
- let borderColorHover = onClick ? primary.light1 : 'transparent';
- let color = grayscale.dark1;
+ let baseColor;
+ if (type === 'default') {
+ baseColor = grayscale;
+ } else if (type === 'success') {
+ baseColor = success;
+ } else if (type === 'alert') {
+ baseColor = alert;
+ } else if (type === 'warning') {
+ baseColor = warning;
+ } else if (type === 'danger') {
+ baseColor = error;
+ } else if (type === 'info') {
+ baseColor = info;
+ } else if (type === 'secondary') {
+ baseColor = secondary;
+ } else {
+ baseColor = primary;
+ }
+ let color = baseColor.dark2;
+ let borderColor = baseColor.light1;
+ let backgroundColor = baseColor.light2;
+ let backgroundColorHover = onClick ? baseColor.light1 : backgroundColor;
+ let borderColorHover = onClick ? baseColor.base : borderColor;
- if (type !== 'default') {
- 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)) {
Review 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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #31575:
URL: https://github.com/apache/superset/pull/31575#discussion_r1905803643
##########
superset-frontend/packages/superset-ui-core/src/style/index.tsx:
##########
@@ -59,14 +59,24 @@ const defaultTheme = {
label: '#879399',
help: '#737373',
},
+ grayscale: {
+ base: '#666666',
+ dark1: '#323232',
+ dark2: '#000000',
+ light1: '#B2B2B2',
+ light2: '#F0F0F0',
+ light3: '#F7F7F7',
+ light4: '#FFFFFF',
Review Comment:
Is it intentional that light4 and light5 are now equal? Same for primary and secondary
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2578901127
Looks like `npm` had an outage earlier, re-triggering eph env
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2578770089
/testenv up
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "mohdslmn (via GitHub)" <gi...@apache.org>.
mohdslmn commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2565022218
ok got it @mistercrunch
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2575859361
@kasiazjc Ephemeral environment spinning up at http://34.209.140.91:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2578913889
@mistercrunch Ephemeral environment spinning up at http://52.27.147.237:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2578901205
/testenv up
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31575:
URL: https://github.com/apache/superset/pull/31575#discussion_r1905981906
##########
superset-frontend/packages/superset-ui-core/src/style/index.tsx:
##########
@@ -59,14 +59,24 @@ const defaultTheme = {
label: '#879399',
help: '#737373',
},
+ grayscale: {
+ base: '#666666',
+ dark1: '#323232',
+ dark2: '#000000',
+ light1: '#B2B2B2',
+ light2: '#F0F0F0',
+ light3: '#F7F7F7',
+ light4: '#FFFFFF',
Review 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....
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2568468935
@kasiazjc looks like this may be ready to go if you want to take a look
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2556046433
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 `/korbit-review`.
> Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2557416789
/testenv up
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2557417037
@mistercrunch Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/12435630775).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "mohdslmn (via GitHub)" <gi...@apache.org>.
mohdslmn commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2564363669
@mistercrunch please let me know, if you can assign this issue to me?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "kasiazjc (via GitHub)" <gi...@apache.org>.
kasiazjc commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2575835425
> Realign labels to look and feel closer to native AntD while integrating the theme colors.
>
> As part of this PR:
>
> * modify 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
>
> ### label storybook
> <img alt="Screenshot 2024-12-19 at 5 01 47 PM" width="1002" src="https://private-user-images.githubusercontent.com/487433/397578074-38c31983-c36e-436d-aaf7-a21e1ba5e4a5.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzYyNzAxOTYsIm5iZiI6MTczNjI2OTg5NiwicGF0aCI6Ii80ODc0MzMvMzk3NTc4MDc0LTM4YzMxOTgzLWMzNmUtNDM2ZC1hYWY3LWEyMWUxYmE1ZTRhNS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMTA3JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDEwN1QxNzExMzZaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT01OTJiODhmM2ZiMDBkZWVkNTBhNjJiZGVjY2VkYjdhMzhiNTUxN2IxMWU3MWViZmI3MDdjNjAyMzIwZTU0ODNlJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.Dr7JmBy_F8qY-cvF5X6yvW5k1OIFaDDqg_OLa6Q9Dww">
> ### dashboard header
> <img alt="Screenshot 2024-12-19 at 5 02 16 PM" width="623" src="https://private-user-images.githubusercontent.com/487433/397578064-237271dc-ca79-4d83-8581-60268e136e1b.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzYyNzAxOTYsIm5iZiI6MTczNjI2OTg5NiwicGF0aCI6Ii80ODc0MzMvMzk3NTc4MDY0LTIzNzI3MWRjLWNhNzktNGQ4My04NTgxLTYwMjY4ZTEzNmUxYi5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMTA3JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDEwN1QxNzExMzZaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT0yNjc4NDZhNTI5ZjIxZWMwMzIxNzFiMDlkNWNmY2M2OTkwOGRhM2Y3MDkyODhkYzc3YzY3MGZhM2YyOWUyZmE1JlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.7jIdprNW6qUfhMUtDNg8qQcfYmw1qO_4Ij40edCSixQ">
> ### crud view datasets
> <img alt="Screenshot 2024-12-19 at 5 02 04 PM" width="903" src="https://private-user-images.githubusercontent.com/487433/397578068-2aa23acb-e6e0-43c1-a47c-56e2f46d3b86.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzYyNzAxOTYsIm5iZiI6MTczNjI2OTg5NiwicGF0aCI6Ii80ODc0MzMvMzk3NTc4MDY4LTJhYTIzYWNiLWU2ZTAtNDNjMS1hNDdjLTU2ZTJmNDZkM2I4Ni5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMTA3JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDEwN1QxNzExMzZaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1jMTA0NTFjNTdmODRjMDI1MzhmNjdjNzM3YjM0YzZlOTEyZTJlNDZmODQxOGUzNDIwZWNlMzI5NGRhZmRmNDMzJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.or88NRR71Abn-QFmol0dFoGF1UiXqV4HJDPIdbrMKQo">
> ### crud view dashboards
> <img alt="Screenshot 2024-12-19 at 5 41 32 PM" width="795" src="https://private-user-images.githubusercontent.com/487433/397587052-b901e439-8479-4b2b-aa4f-5655f11bf15b.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzYyNzAxOTYsIm5iZiI6MTczNjI2OTg5NiwicGF0aCI6Ii80ODc0MzMvMzk3NTg3MDUyLWI5MDFlNDM5LTg0NzktNGIyYi1hYTRmLTU2NTVmMTFiZjE1Yi5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMTA3JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDEwN1QxNzExMzZaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1hY2EzODcyNmU1ZTllMmZhNDhiOTAxZGVkODYxMDI0ZjQ0ZjlkNGVmNzljMTQ1ZmY1NzZhMjhjYjZjMTZmZmMzJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9._ntitSDnsoTlxf1_LDb8iO5fW1v1ktGjPpi9VtwnzSY">
> ### explore
> <img alt="Screenshot 2024-12-19 at 5 41 54 PM" width="535" src="https://private-user-images.githubusercontent.com/487433/397587050-897cbd88-0a02-4d2c-bb35-3988af4901cb.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzYyNzAxOTYsIm5iZiI6MTczNjI2OTg5NiwicGF0aCI6Ii80ODc0MzMvMzk3NTg3MDUwLTg5N2NiZDg4LTBhMDItNGQyYy1iYjM1LTM5ODhhZjQ5MDFjYi5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMTA3JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDEwN1QxNzExMzZaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT05ZDBmMTZhZWUwNWI4YzE0NmE5NzUzMWFlYmYyNTIzMjY4NzdiMmMwOTVjNmRhZDk3OTNhMzNmY2UxZGVjM2I1JlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.YOIbcZn0mwxMGlSPBhN76OSUtp6hoMgO-iTlY5NxRy0">
Love the change! One thing I would suggest is to keep green for published/checked/done etc, and use some other color (secondary?) for virtual datasets.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2576494791
@kgabryje @geido thanks for the review, realizing now that the PR wasn't really settled on the commit you reviewed. Should be much better now. Let me know if I can improve anything.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #31575:
URL: https://github.com/apache/superset/pull/31575#discussion_r1908620818
##########
superset-frontend/src/components/Label/index.tsx:
##########
@@ -71,49 +75,57 @@ export default function Label(props: LabelProps) {
info,
} = colors;
- let backgroundColor = grayscale.light3;
- let backgroundColorHover = onClick ? primary.light2 : grayscale.light3;
- let borderColor = onClick ? grayscale.light2 : 'transparent';
- let borderColorHover = onClick ? primary.light1 : 'transparent';
- let color = grayscale.dark1;
+ let baseColor;
+ if (type === 'default') {
+ baseColor = grayscale;
+ } else if (type === 'success') {
+ baseColor = success;
+ } else if (type === 'alert') {
+ baseColor = alert;
+ } else if (type === 'warning') {
+ baseColor = warning;
+ } else if (type === 'danger') {
+ baseColor = error;
+ } else if (type === 'info') {
+ baseColor = info;
+ } else if (type === 'secondary') {
+ baseColor = secondary;
+ } else {
+ baseColor = primary;
+ }
+ let color = baseColor.dark2;
+ let borderColor = baseColor.light1;
+ let backgroundColor = baseColor.light2;
+ let backgroundColorHover = onClick ? baseColor.light1 : backgroundColor;
+ let borderColorHover = onClick ? baseColor.base : borderColor;
- if (type !== 'default') {
- 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)) {
Review Comment:
Alright looks good for now 👍
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "kasiazjc (via GitHub)" <gi...@apache.org>.
kasiazjc commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2580121114
@mistercrunch yay, was able to test in the test env! 🎉
Two things from me:
- I would keep green, yellow and red for success, warning, error (as they are pretty intuitive) and for other things that need a label use different colors/shades. So I would suggest changing Virtual to for example secondary color.
- found one thing to fix: in database list virtual doesn't have a hover state, and physical has - it doesn't have an action, so I guess we need to get rid of hover
https://github.com/user-attachments/assets/a8eb675a-a29d-4720-95d6-e00977f65240
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch merged PR #31575:
URL: https://github.com/apache/superset/pull/31575
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2557428732
@mistercrunch Ephemeral environment spinning up at http://54.244.170.62:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31575:
URL: https://github.com/apache/superset/pull/31575#discussion_r1905925936
##########
superset-frontend/packages/superset-ui-core/src/style/index.tsx:
##########
@@ -59,14 +59,24 @@ const defaultTheme = {
label: '#879399',
help: '#737373',
},
+ grayscale: {
+ base: '#666666',
+ dark1: '#323232',
+ dark2: '#000000',
+ light1: '#B2B2B2',
+ light2: '#F0F0F0',
+ light3: '#F7F7F7',
+ light4: '#FFFFFF',
Review Comment:
oops I think I had spillover / PR overflow from my larger theming PR, let me take this out
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #31575:
URL: https://github.com/apache/superset/pull/31575#discussion_r1905802639
##########
superset-frontend/src/components/CachedLabel/index.tsx:
##########
@@ -34,7 +35,7 @@ const CacheLabel: FC<CacheLabelProps> = ({
onClick,
cachedTimestamp,
}) => {
- const [hovered, setHovered] = useState(false);
+ const [hovered] = useState(false);
Review Comment:
Looks like this state variable is redundant now?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "kasiazjc (via GitHub)" <gi...@apache.org>.
kasiazjc commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2575835791
/testenv up
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31575:
URL: https://github.com/apache/superset/pull/31575#discussion_r1907750416
##########
superset-frontend/packages/superset-ui-demo/storybook/stories/superset-ui-style/Theme.stories.tsx:
##########
@@ -25,23 +25,96 @@ export default {
export const ThemeColors = () => {
const { colors } = supersetTheme;
- return Object.keys(colors).map(collection => (
+
Review 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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2578901376
@mistercrunch Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/12681118461).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #31575:
URL: https://github.com/apache/superset/pull/31575#discussion_r1908598879
##########
superset-frontend/src/components/Label/index.tsx:
##########
@@ -25,6 +25,8 @@ import {
import { Tag } from 'src/components';
Review 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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2581328245
@kasiazjc thanks for the input, moved to using primary/secondary and fixed the hovered state on non-clickable
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2557459907
@kasiazjc you might be interested in this
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #31575:
URL: https://github.com/apache/superset/pull/31575#discussion_r1905804666
##########
superset-frontend/src/components/Label/Label.test.tsx:
##########
@@ -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,
Review Comment:
Can we use a named constant for that `+4`?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2575836262
@kasiazjc Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/12656053776).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #31575:
URL: https://github.com/apache/superset/pull/31575#discussion_r1905816267
##########
superset-frontend/src/components/Label/index.tsx:
##########
@@ -25,6 +25,8 @@ import {
import { Tag } from 'src/components';
Review 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?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #31575:
URL: https://github.com/apache/superset/pull/31575#issuecomment-2576497381
## latest storybook screenshots
<img width="1469" alt="Screenshot 2025-01-07 at 4 51 38 PM" src="https://github.com/user-attachments/assets/4ea7e7ea-59ff-4339-a24a-1f9c1b080306" />
<img width="1130" alt="Screenshot 2025-01-07 at 4 51 26 PM" src="https://github.com/user-attachments/assets/48b3194d-929e-4659-a0eb-f7729351ebbe" />
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31575:
URL: https://github.com/apache/superset/pull/31575#discussion_r1905925936
##########
superset-frontend/packages/superset-ui-core/src/style/index.tsx:
##########
@@ -59,14 +59,24 @@ const defaultTheme = {
label: '#879399',
help: '#737373',
},
+ grayscale: {
+ base: '#666666',
+ dark1: '#323232',
+ dark2: '#000000',
+ light1: '#B2B2B2',
+ light2: '#F0F0F0',
+ light3: '#F7F7F7',
+ light4: '#FFFFFF',
Review Comment:
oops I think I had spillover / PR overflow from my larger theming PR, let me take this out
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: redesign labels [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #31575:
URL: https://github.com/apache/superset/pull/31575#discussion_r1907740212
##########
superset-frontend/src/components/Label/index.tsx:
##########
@@ -25,6 +25,8 @@ import {
import { Tag } from 'src/components';
Review 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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org