korbit-ai[bot] commented on code in PR #34017:
URL: https://github.com/apache/superset/pull/34017#discussion_r2182928292
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Grid/controlPanel.ts:
##########
@@ -53,7 +56,11 @@ const config: ControlPanelConfig = {
controlSetRows: [
[mapboxStyle],
[viewport],
- ['color_scheme'],
+ ...generateDeckGLColorSchemeControls({
+ defaultSchemeType: COLOR_SCHEME_TYPES.categorical_palette,
+ disableCategoricalColumn: true,
Review Comment:
### Ambiguous boolean parameter name <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The boolean parameter name is ambiguous and doesn't clearly indicate its
purpose.
###### Why this matters
The parameter name could mean either disabling a column that is categorical
or disabling the categorization of a column. This ambiguity forces developers
to look up the implementation details.
###### Suggested change ∙ *Feature Preview*
Either rename the parameter to be more specific or add a clarifying comment:
```typescript
// Disable the option to select a categorical column since grid layers use
numeric aggregations
disableCategoricalColumn: true,
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/be2a2578-7a8c-489c-b4a2-2a77567cb90b/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/be2a2578-7a8c-489c-b4a2-2a77567cb90b?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/be2a2578-7a8c-489c-b4a2-2a77567cb90b?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/be2a2578-7a8c-489c-b4a2-2a77567cb90b?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/be2a2578-7a8c-489c-b4a2-2a77567cb90b)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6ae5ea77-5f7b-480b-a87e-8aa963f85364 -->
[](6ae5ea77-5f7b-480b-a87e-8aa963f85364)
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Heatmap/controlPanel.ts:
##########
@@ -99,7 +102,24 @@ const config: ControlPanelConfig = {
controlSetRows: [
[mapboxStyle],
[viewport],
- ['linear_color_scheme'],
+ [
+ {
+ name: 'color_scheme_type',
+ config: {
+ type: 'SelectControl',
+ label: t('Color Scheme Type'),
+ clearable: false,
+ validators: [],
+ choices: [
+ [COLOR_SCHEME_TYPES.fixed_color, t('Fixed color')],
+ [COLOR_SCHEME_TYPES.linear_palette, t('Linear palette')],
+ ],
+ default: COLOR_SCHEME_TYPES.linear_palette,
+ },
Review Comment:
### Missing color scheme type usage guidance <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The 'color_scheme_type' control lacks a description explaining when to use
each color scheme type.
###### Why this matters
Users may select the wrong color scheme type for their visualization needs,
leading to suboptimal data representation.
###### Suggested change ∙ *Feature Preview*
Add a description field:
```typescript
description: t('Choose Fixed color for single color intensity visualization,
or Linear palette for a gradient effect'),
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cead173f-737e-4a46-934d-6dfbea327e12/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cead173f-737e-4a46-934d-6dfbea327e12?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cead173f-737e-4a46-934d-6dfbea327e12?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cead173f-737e-4a46-934d-6dfbea327e12?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cead173f-737e-4a46-934d-6dfbea327e12)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:26a5d163-aeef-4638-8190-4dd7fe097940 -->
[](26a5d163-aeef-4638-8190-4dd7fe097940)
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Grid/Grid.tsx:
##########
@@ -69,19 +73,33 @@
data = jsFnMutator(data);
}
+ const colorSchemeType = getSelectedColorSchemeType(fd);
+ const colorRange = getColorRange(fd, colorSchemeType, colorScale);
+
+ const colorBreakpoints = fd.color_breakpoints;
+
const aggFunc = getAggFunc(fd.js_agg_function, p => p.weight);
+ const colorAggFunc =
+ colorSchemeType === COLOR_SCHEME_TYPES.color_breakpoints
+ ? (p: number[]) => getColorForBreakpoints(aggFunc, p, colorBreakpoints)
+ : aggFunc;
Review Comment:
### Mixed Layer and Aggregation Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The aggregation function logic is mixed with layer configuration, violating
the Single Responsibility Principle.
###### Why this matters
This mixing of concerns makes the code harder to maintain and test, and
makes it difficult to modify either the aggregation logic or layer
configuration independently.
###### Suggested change ∙ *Feature Preview*
Extract aggregation logic into a separate function:
```typescript
function createAggregationFunctions(formData, colorSchemeType,
colorBreakpoints) {
const baseAggFunc = getAggFunc(formData.js_agg_function, p => p.weight);
return {
elevationAggFunc: baseAggFunc,
colorAggFunc: colorSchemeType === COLOR_SCHEME_TYPES.color_breakpoints
? (p: number[]) => getColorForBreakpoints(baseAggFunc, p,
colorBreakpoints)
: baseAggFunc
};
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2425204c-39bc-4eb6-adbc-0bdca81b0b95/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2425204c-39bc-4eb6-adbc-0bdca81b0b95?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2425204c-39bc-4eb6-adbc-0bdca81b0b95?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2425204c-39bc-4eb6-adbc-0bdca81b0b95?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2425204c-39bc-4eb6-adbc-0bdca81b0b95)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:e07345cf-5c6d-4cc4-a644-64c0268a92b7 -->
[](e07345cf-5c6d-4cc4-a644-64c0268a92b7)
##########
superset-frontend/src/explore/components/controls/ColorBreakpointsControl/index.tsx:
##########
@@ -0,0 +1,127 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with work for additional information
+ * regarding copyright ownership. The ASF licenses file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { useState, useEffect } from 'react';
+import { styled, t } from '@superset-ui/core';
+import DndSelectLabel from
'src/explore/components/controls/DndColumnSelectControl/DndSelectLabel';
+import ColorBreakpointOption from './ColorBreakpointOption';
+import { ColorBreakpointType, ColorBreakpointsControlProps } from './types';
+import ColorBreakpointPopoverTrigger from './ColorBreakpointPopoverTrigger';
+
+const DEFAULT_COLOR_BREAKPOINTS: ColorBreakpointType[] = [];
+
+const NewContourFormatPlaceholder = styled('div')`
+ position: relative;
+ width: calc(100% - ${({ theme }) => theme.sizeUnit}px);
+ bottom: ${({ theme }) => theme.sizeUnit * 4}px;
+ left: 0;
+`;
+
+const ColorBreakpointsControl = ({
+ onChange,
+ ...props
+}: ColorBreakpointsControlProps) => {
+ const [popoverVisible, setPopoverVisible] = useState(false);
+ const [colorBreakpoints, setColorBreakpoints] = useState<
+ ColorBreakpointType[]
+ >(props?.value ? props?.value : DEFAULT_COLOR_BREAKPOINTS);
+
+ useEffect(() => {
+ onChange?.(colorBreakpoints);
+ }, [colorBreakpoints, onChange]);
+
+ const togglePopover = (visible: boolean) => {
+ setPopoverVisible(visible);
+ };
+
+ const handleClickGhostButton = () => {
+ togglePopover(true);
+ };
+
+ const saveColorBreakpoint = (breakpoint: ColorBreakpointType) => {
+ setColorBreakpoints([
+ ...colorBreakpoints,
+ {
+ ...breakpoint,
+ id: colorBreakpoints.length,
+ },
+ ]);
+ togglePopover(false);
+ };
+
+ const removeColorBreakpoint = (index: number) => {
+ const newBreakpoints = [...colorBreakpoints];
+ newBreakpoints.splice(index, 1);
+ setColorBreakpoints(newBreakpoints);
+ };
+
+ const editColorBreakpoint = (
+ breakpoint: ColorBreakpointType,
+ index: number,
+ ) => {
+ const newBreakpoints = [...colorBreakpoints];
+ newBreakpoints[index] = {
+ ...breakpoint,
+ id: index,
+ };
+ setColorBreakpoints(newBreakpoints);
+ };
+
+ const valuesRenderer = () =>
+ colorBreakpoints.map((breakpoint, index) => (
+ <ColorBreakpointOption
+ key={index}
+ saveColorBreakpoint={(newBreakpoint: ColorBreakpointType) =>
+ editColorBreakpoint(newBreakpoint, index)
+ }
+ breakpoint={breakpoint}
+ colorBreakpoints={colorBreakpoints}
+ index={index}
+ onClose={removeColorBreakpoint}
+ onShift={() => {}}
Review Comment:
### Unexplained Empty Function Prop <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Empty function prop being passed without explanation of why it's needed or
if it's a required prop.
###### Why this matters
Empty function props without context make code harder to understand and
maintain, leaving developers unsure if the empty implementation is intentional
or a missing feature.
###### Suggested change ∙ *Feature Preview*
Either remove if not needed or add implementation:
```typescript
// Remove if not needed
<ColorBreakpointOption
// ... other props
// onShift prop removed
/>
// Or implement if needed
onShift={(direction: 'up' | 'down') => handleBreakpointShift(index,
direction)}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/00fd87c8-940b-4605-96d8-742fec6d9779/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/00fd87c8-940b-4605-96d8-742fec6d9779?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/00fd87c8-940b-4605-96d8-742fec6d9779?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/00fd87c8-940b-4605-96d8-742fec6d9779?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/00fd87c8-940b-4605-96d8-742fec6d9779)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9ad0e89c-a9bf-4def-93f9-e1e6f55afe1d -->
[](9ad0e89c-a9bf-4def-93f9-e1e6f55afe1d)
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Grid/Grid.tsx:
##########
@@ -69,19 +73,33 @@ export function getLayer(
data = jsFnMutator(data);
}
+ const colorSchemeType = getSelectedColorSchemeType(fd);
+ const colorRange = getColorRange(fd, colorSchemeType, colorScale);
+
+ const colorBreakpoints = fd.color_breakpoints;
+
const aggFunc = getAggFunc(fd.js_agg_function, p => p.weight);
+ const colorAggFunc =
+ colorSchemeType === COLOR_SCHEME_TYPES.color_breakpoints
+ ? (p: number[]) => getColorForBreakpoints(aggFunc, p, colorBreakpoints)
+ : aggFunc;
Review Comment:
### Missing Color Breakpoints Validation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The colorAggFunc assignment doesn't handle the case where colorBreakpoints
is undefined, which could occur if the form data doesn't include color
breakpoints.
###### Why this matters
This could lead to runtime errors when trying to use the color breakpoints
feature if the form data is incomplete or improperly initialized.
###### Suggested change ∙ *Feature Preview*
Add validation to ensure colorBreakpoints exists before using it:
```typescript
const colorAggFunc =
colorSchemeType === COLOR_SCHEME_TYPES.color_breakpoints &&
colorBreakpoints
? (p: number[]) => getColorForBreakpoints(aggFunc, p, colorBreakpoints)
: aggFunc;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/95a18e3f-3674-4aab-a23c-6f2563f3a62a/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/95a18e3f-3674-4aab-a23c-6f2563f3a62a?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/95a18e3f-3674-4aab-a23c-6f2563f3a62a?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/95a18e3f-3674-4aab-a23c-6f2563f3a62a?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/95a18e3f-3674-4aab-a23c-6f2563f3a62a)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:059e493b-4df8-44df-bfde-792133602c10 -->
[](059e493b-4df8-44df-bfde-792133602c10)
##########
superset-frontend/src/explore/components/controls/ColorBreakpointsControl/types.ts:
##########
@@ -0,0 +1,76 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { ReactNode } from 'react';
+import { OptionValueType } from
'src/explore/components/controls/DndColumnSelectControl/types';
+import { ControlComponentProps } from 'src/explore/components/Control';
+
+export interface ColorType {
+ r: number;
+ g: number;
+ b: number;
+ a: number;
+}
+
+export interface ColorBreakpointType {
+ id?: number;
+ color?: ColorType;
+ minValue?: number;
+ maxValue?: number;
+}
+
+export interface ErrorMapType {
+ color: string[];
+ minValue: string[];
+ maxValue: string[];
+}
+
+export interface ColorBreakpointsControlProps
+ extends ControlComponentProps<OptionValueType[]> {
+ breakpoints: ColorBreakpointType[];
+}
+
+export interface ColorBreakpointsPopoverTriggerProps {
+ description?: string;
+ hovered?: boolean;
+ value?: ColorBreakpointType;
+ children?: ReactNode;
+ saveColorBreakpoint: (colorBreakpoint: ColorBreakpointType) => void;
+ isControlled?: boolean;
+ visible?: boolean;
+ toggleVisibility?: (visibility: boolean) => void;
+ colorBreakpoints: ColorBreakpointType[];
+}
+
+export interface ColorBreakpointsPopoverControlProps {
+ description?: string;
+ hovered?: boolean;
+ value?: ColorBreakpointType;
+ onSave?: (colorBreakpoint: ColorBreakpointType) => void;
+ onClose?: () => void;
+ colorBreakpoints: ColorBreakpointType[];
+}
Review Comment:
### Redundant Interface Properties <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The interfaces ColorBreakpointsPopoverTriggerProps and
ColorBreakpointsPopoverControlProps share many common properties, violating the
DRY principle.
###### Why this matters
Duplicated interface properties increase maintenance overhead and risk of
inconsistencies when changes are needed.
###### Suggested change ∙ *Feature Preview*
Extract common properties into a base interface:
```typescript
interface BaseColorBreakpointsPopoverProps {
description?: string;
hovered?: boolean;
value?: ColorBreakpointType;
colorBreakpoints: ColorBreakpointType[];
}
export interface ColorBreakpointsPopoverTriggerProps extends
BaseColorBreakpointsPopoverProps {
children?: ReactNode;
saveColorBreakpoint: (colorBreakpoint: ColorBreakpointType) => void;
isControlled?: boolean;
visible?: boolean;
toggleVisibility?: (visibility: boolean) => void;
}
export interface ColorBreakpointsPopoverControlProps extends
BaseColorBreakpointsPopoverProps {
onSave?: (colorBreakpoint: ColorBreakpointType) => void;
onClose?: () => void;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/273d8047-8024-450f-941f-0107bbde3285/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/273d8047-8024-450f-941f-0107bbde3285?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/273d8047-8024-450f-941f-0107bbde3285?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/273d8047-8024-450f-941f-0107bbde3285?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/273d8047-8024-450f-941f-0107bbde3285)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9c729085-c310-49a5-9f85-eb0ed8f7b514 -->
[](9c729085-c310-49a5-9f85-eb0ed8f7b514)
##########
superset-frontend/src/explore/components/controls/ColorBreakpointsControl/index.tsx:
##########
@@ -0,0 +1,127 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with work for additional information
+ * regarding copyright ownership. The ASF licenses file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { useState, useEffect } from 'react';
+import { styled, t } from '@superset-ui/core';
+import DndSelectLabel from
'src/explore/components/controls/DndColumnSelectControl/DndSelectLabel';
+import ColorBreakpointOption from './ColorBreakpointOption';
+import { ColorBreakpointType, ColorBreakpointsControlProps } from './types';
+import ColorBreakpointPopoverTrigger from './ColorBreakpointPopoverTrigger';
+
+const DEFAULT_COLOR_BREAKPOINTS: ColorBreakpointType[] = [];
+
+const NewContourFormatPlaceholder = styled('div')`
Review Comment:
### Misleading Component Name <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The component name 'NewContourFormatPlaceholder' is misleading as it's not
related to contour formatting but rather serves as a placeholder for color
breakpoint UI.
###### Why this matters
Misleading component names can cause confusion and make the code harder to
maintain as developers might expect different functionality based on the name.
###### Suggested change ∙ *Feature Preview*
```typescript
const ColorBreakpointPlaceholder = styled('div')`
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6a3b07c8-476d-496c-8073-6fba7dc3e4f6/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6a3b07c8-476d-496c-8073-6fba7dc3e4f6?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6a3b07c8-476d-496c-8073-6fba7dc3e4f6?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6a3b07c8-476d-496c-8073-6fba7dc3e4f6?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6a3b07c8-476d-496c-8073-6fba7dc3e4f6)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:625dfa2e-94de-4d44-9c28-5480afce6b1b -->
[](625dfa2e-94de-4d44-9c28-5480afce6b1b)
##########
superset-frontend/src/explore/components/controls/ColorBreakpointsControl/ColorBreakpointPopoverTrigger.tsx:
##########
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useState } from 'react';
+import ControlPopover from '../ControlPopover/ControlPopover';
+import { ColorBreakpointsPopoverTriggerProps } from './types';
+import ColorBreakpointPopoverControl from './ColorBreakpointPopoverControl';
+
+const ColorBreakpointsPopoverTrigger = ({
+ value: initialValue,
+ saveColorBreakpoint,
+ isControlled,
+ visible: controlledVisibility,
+ toggleVisibility,
+ colorBreakpoints,
+ ...props
+}: ColorBreakpointsPopoverTriggerProps) => {
+ const [isVisible, setIsVisible] = useState(false);
+
+ const visible = isControlled ? controlledVisibility : isVisible;
+ const setVisibility =
+ isControlled && toggleVisibility ? toggleVisibility : setIsVisible;
+
+ const popoverContent = (
+ <ColorBreakpointPopoverControl
+ value={initialValue}
+ colorBreakpoints={colorBreakpoints}
+ onSave={saveColorBreakpoint}
+ onClose={() => setVisibility(false)}
+ />
+ );
+
+ return (
+ <ControlPopover
+ trigger="click"
+ content={popoverContent}
+ defaultOpen={visible}
+ open={visible}
Review Comment:
### Conflicting popover control props <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Redundant usage of both defaultOpen and open props in ControlPopover which
can lead to conflicting behavior.
###### Why this matters
Using both controlled (open) and uncontrolled (defaultOpen) props
simultaneously can cause the component to behave unpredictably as they serve
different purposes.
###### Suggested change ∙ *Feature Preview*
Remove the defaultOpen prop as it's not needed when using controlled open
prop:
```typescript
<ControlPopover
trigger="click"
content={popoverContent}
open={visible}
onOpenChange={setVisibility}
destroyOnHidden
>
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/81dc6e2e-26b5-4228-9013-957d8f1a03d5/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/81dc6e2e-26b5-4228-9013-957d8f1a03d5?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/81dc6e2e-26b5-4228-9013-957d8f1a03d5?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/81dc6e2e-26b5-4228-9013-957d8f1a03d5?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/81dc6e2e-26b5-4228-9013-957d8f1a03d5)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:2e7be88a-b382-49f7-8cef-2ca820d7dfd5 -->
[](2e7be88a-b382-49f7-8cef-2ca820d7dfd5)
##########
superset-frontend/src/explore/components/controls/ColorBreakpointsControl/index.tsx:
##########
@@ -0,0 +1,127 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with work for additional information
+ * regarding copyright ownership. The ASF licenses file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { useState, useEffect } from 'react';
+import { styled, t } from '@superset-ui/core';
+import DndSelectLabel from
'src/explore/components/controls/DndColumnSelectControl/DndSelectLabel';
+import ColorBreakpointOption from './ColorBreakpointOption';
+import { ColorBreakpointType, ColorBreakpointsControlProps } from './types';
+import ColorBreakpointPopoverTrigger from './ColorBreakpointPopoverTrigger';
+
+const DEFAULT_COLOR_BREAKPOINTS: ColorBreakpointType[] = [];
+
+const NewContourFormatPlaceholder = styled('div')`
+ position: relative;
+ width: calc(100% - ${({ theme }) => theme.sizeUnit}px);
+ bottom: ${({ theme }) => theme.sizeUnit * 4}px;
+ left: 0;
+`;
+
+const ColorBreakpointsControl = ({
+ onChange,
+ ...props
+}: ColorBreakpointsControlProps) => {
+ const [popoverVisible, setPopoverVisible] = useState(false);
+ const [colorBreakpoints, setColorBreakpoints] = useState<
+ ColorBreakpointType[]
+ >(props?.value ? props?.value : DEFAULT_COLOR_BREAKPOINTS);
+
+ useEffect(() => {
+ onChange?.(colorBreakpoints);
+ }, [colorBreakpoints, onChange]);
+
+ const togglePopover = (visible: boolean) => {
+ setPopoverVisible(visible);
+ };
+
+ const handleClickGhostButton = () => {
+ togglePopover(true);
+ };
+
+ const saveColorBreakpoint = (breakpoint: ColorBreakpointType) => {
+ setColorBreakpoints([
+ ...colorBreakpoints,
+ {
+ ...breakpoint,
+ id: colorBreakpoints.length,
+ },
+ ]);
+ togglePopover(false);
+ };
+
+ const removeColorBreakpoint = (index: number) => {
+ const newBreakpoints = [...colorBreakpoints];
+ newBreakpoints.splice(index, 1);
+ setColorBreakpoints(newBreakpoints);
+ };
+
+ const editColorBreakpoint = (
+ breakpoint: ColorBreakpointType,
+ index: number,
+ ) => {
+ const newBreakpoints = [...colorBreakpoints];
+ newBreakpoints[index] = {
+ ...breakpoint,
+ id: index,
+ };
+ setColorBreakpoints(newBreakpoints);
+ };
+
+ const valuesRenderer = () =>
+ colorBreakpoints.map((breakpoint, index) => (
+ <ColorBreakpointOption
+ key={index}
+ saveColorBreakpoint={(newBreakpoint: ColorBreakpointType) =>
+ editColorBreakpoint(newBreakpoint, index)
+ }
+ breakpoint={breakpoint}
+ colorBreakpoints={colorBreakpoints}
+ index={index}
+ onClose={removeColorBreakpoint}
+ onShift={() => {}}
+ />
+ ));
+
+ const ghostButtonText = t('Click to add new breakpoint');
+
+ return (
+ <>
+ <DndSelectLabel
+ onDrop={() => {}}
+ canDrop={() => true}
Review Comment:
### Incomplete Drag and Drop Implementation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The DndSelectLabel component's drag and drop functionality is not properly
implemented, with an empty onDrop handler and a canDrop that always returns
true.
###### Why this matters
Without proper drag and drop implementation, users cannot drag values from
other controls or reorder items using drag and drop, limiting the intended
functionality of the DndSelectLabel component.
###### Suggested change ∙ *Feature Preview*
Implement proper drag and drop handlers:
```typescript
const handleDrop = (item: any) => {
// Handle dropped items based on your requirements
if (item.value) {
const newBreakpoint = createBreakpointFromDrop(item);
saveColorBreakpoint(newBreakpoint);
}
};
const canDrop = (item: any) => {
// Add validation logic
return item?.type === 'column' && !colorBreakpoints.find(bp => bp.column
=== item.value);
};
// In JSX:
<DndSelectLabel
onDrop={handleDrop}
canDrop={canDrop}
// ... other props
/>
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fb669a0c-3009-457b-87de-3facf8939423/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fb669a0c-3009-457b-87de-3facf8939423?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fb669a0c-3009-457b-87de-3facf8939423?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fb669a0c-3009-457b-87de-3facf8939423?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fb669a0c-3009-457b-87de-3facf8939423)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:c9c98f3b-13a5-4dba-b6ab-8fbbd5eccecf -->
[](c9c98f3b-13a5-4dba-b6ab-8fbbd5eccecf)
##########
superset-frontend/src/explore/components/controls/ColorBreakpointsControl/index.tsx:
##########
@@ -0,0 +1,127 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with work for additional information
+ * regarding copyright ownership. The ASF licenses file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { useState, useEffect } from 'react';
+import { styled, t } from '@superset-ui/core';
+import DndSelectLabel from
'src/explore/components/controls/DndColumnSelectControl/DndSelectLabel';
+import ColorBreakpointOption from './ColorBreakpointOption';
+import { ColorBreakpointType, ColorBreakpointsControlProps } from './types';
+import ColorBreakpointPopoverTrigger from './ColorBreakpointPopoverTrigger';
+
+const DEFAULT_COLOR_BREAKPOINTS: ColorBreakpointType[] = [];
+
+const NewContourFormatPlaceholder = styled('div')`
+ position: relative;
+ width: calc(100% - ${({ theme }) => theme.sizeUnit}px);
+ bottom: ${({ theme }) => theme.sizeUnit * 4}px;
+ left: 0;
+`;
+
+const ColorBreakpointsControl = ({
+ onChange,
+ ...props
+}: ColorBreakpointsControlProps) => {
+ const [popoverVisible, setPopoverVisible] = useState(false);
+ const [colorBreakpoints, setColorBreakpoints] = useState<
+ ColorBreakpointType[]
+ >(props?.value ? props?.value : DEFAULT_COLOR_BREAKPOINTS);
+
+ useEffect(() => {
+ onChange?.(colorBreakpoints);
+ }, [colorBreakpoints, onChange]);
+
+ const togglePopover = (visible: boolean) => {
+ setPopoverVisible(visible);
+ };
+
+ const handleClickGhostButton = () => {
+ togglePopover(true);
+ };
+
+ const saveColorBreakpoint = (breakpoint: ColorBreakpointType) => {
+ setColorBreakpoints([
+ ...colorBreakpoints,
+ {
+ ...breakpoint,
+ id: colorBreakpoints.length,
+ },
+ ]);
+ togglePopover(false);
+ };
+
+ const removeColorBreakpoint = (index: number) => {
+ const newBreakpoints = [...colorBreakpoints];
+ newBreakpoints.splice(index, 1);
+ setColorBreakpoints(newBreakpoints);
+ };
+
+ const editColorBreakpoint = (
+ breakpoint: ColorBreakpointType,
+ index: number,
+ ) => {
+ const newBreakpoints = [...colorBreakpoints];
+ newBreakpoints[index] = {
+ ...breakpoint,
+ id: index,
+ };
+ setColorBreakpoints(newBreakpoints);
+ };
Review Comment:
### Array Indices Used As IDs <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The component uses array indices as unique identifiers for breakpoints,
which is fragile and violates good design practices for managing collections of
items.
###### Why this matters
Using array indices as IDs can lead to incorrect state updates and bugs when
items are reordered or deleted, as the IDs will change based on position rather
than identity.
###### Suggested change ∙ *Feature Preview*
Use a unique identifier generator or timestamp for IDs:
```typescript
const editColorBreakpoint = (breakpoint: ColorBreakpointType, index: number)
=> {
const newBreakpoints = [...colorBreakpoints];
newBreakpoints[index] = {
...breakpoint,
id: generateUniqueId(), // Use UUID or another unique ID generator
};
setColorBreakpoints(newBreakpoints);
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9251105a-2497-4108-8876-cea918f3af88/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9251105a-2497-4108-8876-cea918f3af88?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9251105a-2497-4108-8876-cea918f3af88?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9251105a-2497-4108-8876-cea918f3af88?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9251105a-2497-4108-8876-cea918f3af88)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:509ccfaa-7ae1-4661-a5e1-908cb0591151 -->
[](509ccfaa-7ae1-4661-a5e1-908cb0591151)
--
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]
