korbit-ai[bot] commented on code in PR #31590:
URL: https://github.com/apache/superset/pull/31590#discussion_r1930006812
##########
superset-frontend/plugins/legacy-plugin-chart-chord/src/ReactChord.jsx:
##########
@@ -44,14 +44,14 @@ export default styled(Chord)`
pointer-events: all;
}
.superset-legacy-chart-chord svg .group path {
- fill-opacity: ${theme.opacity.mediumHeavy};
+ fill-opacity: 60%;
}
.superset-legacy-chart-chord svg path.chord {
- stroke: ${theme.colors.grayscale.dark2};
+ stroke: ${theme.colorText};
stroke-width: 0.25px;
}
.superset-legacy-chart-chord svg #circle:hover path.fade {
- opacity: ${theme.opacity.light};
+ opacity: 10%;
Review Comment:
### Inconsistent Hover State Opacity <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Direct percentage value for opacity on hover state doesn't align with the
theming system and may cause inconsistent visual feedback.
###### Why this matters
User interaction feedback should be consistent across the application. Using
hardcoded values instead of theme variables breaks this consistency and could
lead to poor user experience.
###### Suggested change ∙ *Feature Preview*
Replace with theme-controlled opacity:
```jsx
opacity: ${theme.opacity.light};
```
</details>
<sub>💡 Does this comment miss the mark? [Tell us
why](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/04804c8e-6e7d-49b3-8c32-b88d3279b150?suggestedFixEnabled=true)
and Korbit will adapt to your team’s feedback.
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:1ea29e08-6383-4c30-89df-e79b45884966 -->
##########
superset-frontend/src/hooks/useJsonTreeTheme.ts:
##########
@@ -20,22 +20,23 @@ import { useTheme } from '@superset-ui/core';
export const useJsonTreeTheme = () => {
const theme = useTheme();
+
return {
- base00: theme.colors.grayscale.dark2,
- base01: theme.colors.grayscale.dark1,
- base02: theme.colors.grayscale.base,
- base03: theme.colors.grayscale.light1,
- base04: theme.colors.grayscale.light2,
- base05: theme.colors.grayscale.light3,
- base06: theme.colors.grayscale.light4,
- base07: theme.colors.grayscale.light5,
- base08: theme.colors.error.base,
- base09: theme.colors.error.light1,
- base0A: theme.colors.error.light2,
- base0B: theme.colors.success.base,
- base0C: theme.colors.primary.light1,
- base0D: theme.colors.primary.base,
- base0E: theme.colors.primary.dark1,
- base0F: theme.colors.error.dark1,
+ base00: theme.colorBgContainer,
+ base01: theme.colorBgLayout,
+ base02: theme.colorBorder,
+ base03: theme.colorBorder,
+ base04: theme.colorText,
+ base05: theme.colorText,
+ base06: theme.colorText,
+ base07: theme.colorText,
Review Comment:
### Multiple identical text colors impair JSON structure visibility
<sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Four consecutive base properties (base04 through base07) are assigned the
same colorText value, eliminating the intended visual progression in the JSON
tree.
###### Why this matters
JSON tree viewers rely on color gradients to show depth and relationships.
Using identical colors for multiple levels makes it difficult for users to
visually parse the JSON structure.
###### Suggested change ∙ *Feature Preview*
Use a progression of related colors for visual hierarchy:
```typescript
base04: theme.colorText,
base05: theme.colorTextSecondary,
base06: theme.colorTextTertiary,
base07: theme.colorTextQuaternary,
```
</details>
<sub>💡 Does this comment miss the mark? [Tell us
why](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/87b02038-4d37-4dd1-bbe8-52a970928246?suggestedFixEnabled=true)
and Korbit will adapt to your team’s feedback.
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:0b2091f5-badb-4833-80bf-6b5e942d51f1 -->
##########
superset-frontend/src/SqlLab/components/SaveDatasetActionButton/index.tsx:
##########
@@ -38,19 +38,19 @@ const SaveDatasetActionButton = ({
DropdownButton as FC<DropdownButtonProps>,
)`
&.ant-dropdown-button button.ant-btn.ant-btn-default {
- font-weight: ${theme.gridUnit * 150};
+ font-weight: ${theme.sizeUnit * 150};
Review Comment:
### Invalid font-weight calculation using sizeUnit <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using sizeUnit (which is meant for spacing/sizing) to calculate font-weight
will likely result in an incorrect or invalid font-weight value.
###### Why this matters
Font-weight values typically range from 100 to 900 in steps of 100. Using a
sizing unit multiplied by 150 could produce values outside this range, leading
to unexpected text rendering.
###### Suggested change ∙ *Feature Preview*
Replace with an appropriate font-weight value or theme variable. For example:
```typescript
font-weight: 600; // or
font-weight: ${theme.typography.weights.bold};
```
</details>
<sub>💡 Does this comment miss the mark? [Tell us
why](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ea91d7c7-55c1-4fac-b316-83ea3ed1fe06?suggestedFixEnabled=true)
and Korbit will adapt to your team’s feedback.
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:bb174bd4-db43-4eee-988b-7ede69b9b2c7 -->
##########
superset-frontend/src/components/DatePicker/DatePicker.stories.tsx:
##########
@@ -87,9 +87,11 @@ InteractiveDatePicker.args = {
InteractiveDatePicker.argTypes = interactiveTypes;
-export const InteractiveRangePicker = (args: RangePickerProps) => (
- <RangePicker {...args} />
-);
+export const InteractiveRangePicker = (
+ args: Omit<RangePickerProps, 'picker'> & {
+ picker?: 'date';
+ },
+) => <RangePicker {...args} />;
Review Comment:
### RangePicker type unnecessarily restricts picker options <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The type definition restricts the RangePicker to only use 'date' as picker
type, which contradicts the picker options defined in interactiveTypes that
include week, month, quarter, and year.
###### Why this matters
Users won't be able to use the RangePicker with other valid picker types
like week, month, quarter, or year, despite these options being available in
the story controls.
###### Suggested change ∙ *Feature Preview*
Remove the type restriction to allow all picker types:
```typescript
export const InteractiveRangePicker = (
args: RangePickerProps,
) => <RangePicker {...args} />;
```
</details>
<sub>💡 Does this comment miss the mark? [Tell us
why](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/75a9ee4e-d075-440c-bf78-bce94ff62847?suggestedFixEnabled=true)
and Korbit will adapt to your team’s feedback.
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:11a14ca4-2cd3-4e88-8674-561b49ccf5c8 -->
##########
superset-frontend/packages/superset-ui-core/src/chart/components/FallbackComponent.tsx:
##########
@@ -18,7 +18,7 @@
*/
import { t } from '@superset-ui/core';
-import { SupersetTheme } from '../../style';
+import { SupersetTheme } from '../..';
Review Comment:
### Use explicit relative import paths. <sub></sub>
<details>
<summary>Tell me more</summary>
​
The import path modification `import { SupersetTheme } from '../..';` is
less intuitive to understand where `SupersetTheme` is being imported from
compared to the previous import statement. A suggestion would be to consider
using a more explicit relative import path that helps other developers track
the module with ease without depending on the context of the code file path.
</details>
<sub>💡 Does this comment miss the mark? [Tell us
why](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ed4161c3-af07-46d3-8702-ca4ca562ebe9?suggestedFixEnabled=true)
and Korbit will adapt to your team’s feedback.
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:a49dbf42-1749-464f-89f6-618711ee01a2 -->
##########
superset-frontend/src/components/Label/reusable/DatasetTypeLabel.tsx:
##########
@@ -36,8 +36,7 @@ const DatasetTypeLabel: React.FC<DatasetTypeLabelProps> = ({
datasetType }) => {
) : (
<Icons.ConsoleSqlOutlined iconSize={SIZE} />
);
- const labelType: 'primary' | 'secondary' =
- datasetType === 'physical' ? 'primary' : 'secondary';
+ const labelType = datasetType === 'physical' ? 'primary' : 'default';
Review Comment:
### Avoid using magic strings in the code. <sub></sub>
<details>
<summary>Tell me more</summary>
​
I suggest avoiding magic strings in your code. The terms 'primary',
'default', 'physical', and 'virtual' seem to be used like constant values.
Instead of repeating these strings throughout your code, it would be best to
define them as constants for clarity and easy of maintenance.
</details>
<sub>💡 Does this comment miss the mark? [Tell us
why](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cacc8938-ea8a-435e-87a3-0b20e72921ce?suggestedFixEnabled=true)
and Korbit will adapt to your team’s feedback.
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:bb66f535-81bc-4d8b-adb1-6c5a698a31cd -->
##########
superset-frontend/src/features/alerts/components/AlertStatusIcon.tsx:
##########
@@ -28,19 +28,17 @@ function getStatusColor(
) {
switch (status) {
case AlertState.Working:
- return theme.colors.primary.base;
+ return theme.colorPrimaryText;
case AlertState.Error:
- return theme.colors.error.base;
+ return theme.colorErrorText;
case AlertState.Success:
- return isReportEnabled
- ? theme.colors.success.base
- : theme.colors.warning.base;
+ return isReportEnabled ? theme.colorSuccessText : theme.colorErrorText;
Review Comment:
### Inconsistent Color for Success State <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The success state shows an error color when isReportEnabled is false, which
is inconsistent with the meaning of a success state.
###### Why this matters
Using an error color for a successful alert trigger will confuse users about
the actual state of the alert, leading to misinterpretation of the alert's
status.
###### Suggested change ∙ *Feature Preview*
Use warning color instead of error color for triggered alerts to maintain
semantic consistency:
```typescript
case AlertState.Success:
return isReportEnabled ? theme.colorSuccessText :
theme.colorWarningText;
```
</details>
<sub>💡 Does this comment miss the mark? [Tell us
why](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1076c844-de5f-42d7-a804-225a73290d1f?suggestedFixEnabled=true)
and Korbit will adapt to your team’s feedback.
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:722cfadf-65a7-4ed5-8a73-25f62ee8aaad -->
--
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]
