korbit-ai[bot] commented on code in PR #33716: URL: https://github.com/apache/superset/pull/33716#discussion_r2133686104
########## superset-frontend/plugins/plugin-chart-echarts/src/Timeline/constants.ts: ########## @@ -0,0 +1,8 @@ +export const ELEMENT_HEIGHT_SCALE = 0.85 as const; Review Comment: ### Unexplained Magic Number <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The magic number 0.85 lacks context about its purpose in scaling element height. ###### Why this matters Without understanding why 0.85 was chosen, future developers may hesitate to modify this value when needed or may misuse it in other contexts. ###### Suggested change ∙ *Feature Preview* ```typescript // Define why 0.85 was chosen for height scaling export const ELEMENT_HEIGHT_SCALE = 0.85 as const; // Provides adequate spacing between timeline elements while maintaining density ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/818a1711-7725-4bd3-be0c-800acc02dd84/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/818a1711-7725-4bd3-be0c-800acc02dd84?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/818a1711-7725-4bd3-be0c-800acc02dd84?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/818a1711-7725-4bd3-be0c-800acc02dd84?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/818a1711-7725-4bd3-be0c-800acc02dd84) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:465cd69e-baf3-4e5e-8811-1b95ec931fed --> [](465cd69e-baf3-4e5e-8811-1b95ec931fed) ########## superset-frontend/src/components/TimePicker/index.tsx: ########## @@ -0,0 +1,31 @@ +/** + * 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 { + TimePicker as AntdTimePicker, + TimePickerProps, + TimeRangePickerProps, +} from 'antd-v5'; + +export const TimePicker = (props: TimePickerProps) => ( + <AntdTimePicker css={{ width: '100%' }} {...props} /> +); Review Comment: ### TimePicker width not configurable <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The hardcoded width: '100%' CSS property may override any width specified in the props, potentially breaking custom width configurations needed for the Timeline Chart visualization. ###### Why this matters Users of the TimePicker component won't be able to customize the width through props, which could lead to layout issues in different contexts of the Timeline Chart. ###### Suggested change ∙ *Feature Preview* Make the width configurable by merging CSS props: ```typescript export const TimePicker = ({ style, ...props }: TimePickerProps) => ( <AntdTimePicker css={{ width: '100%', ...style }} {...props} /> ); ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/91804b20-3c58-48ee-9837-2d564ddb4ed8/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/91804b20-3c58-48ee-9837-2d564ddb4ed8?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/91804b20-3c58-48ee-9837-2d564ddb4ed8?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/91804b20-3c58-48ee-9837-2d564ddb4ed8?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/91804b20-3c58-48ee-9837-2d564ddb4ed8) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:5046e43c-1fc8-4ea4-ac26-0de2ec092340 --> [](5046e43c-1fc8-4ea4-ac26-0de2ec092340) ########## superset-frontend/src/components/TimePicker/index.tsx: ########## @@ -0,0 +1,31 @@ +/** + * 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 { + TimePicker as AntdTimePicker, + TimePickerProps, + TimeRangePickerProps, +} from 'antd-v5'; + +export const TimePicker = (props: TimePickerProps) => ( + <AntdTimePicker css={{ width: '100%' }} {...props} /> Review Comment: ### Inline CSS Should Be In Styled Components Or Theme <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Hardcoded CSS values directly in components reduces readability and maintainability. ###### Why this matters Inline styles make it harder to maintain consistent styling across components and require developers to search through component code rather than a centralized styling file. ###### Suggested change ∙ *Feature Preview* Extract the common styling to a styled component or theme constant: ```typescript const commonPickerStyles = { width: '100%' }; export const TimePicker = (props: TimePickerProps) => ( <AntdTimePicker css={commonPickerStyles} {...props} /> ); export const TimeRangePicker = (props: TimeRangePickerProps) => ( <AntdTimePicker.RangePicker css={commonPickerStyles} {...props} /> ); ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/44695817-7937-431b-a1c3-c0b9e8c494d7/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/44695817-7937-431b-a1c3-c0b9e8c494d7?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/44695817-7937-431b-a1c3-c0b9e8c494d7?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/44695817-7937-431b-a1c3-c0b9e8c494d7?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/44695817-7937-431b-a1c3-c0b9e8c494d7) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:8ea7cf55-e69a-4a5a-9aa7-0bcdf97ca9d6 --> [](8ea7cf55-e69a-4a5a-9aa7-0bcdf97ca9d6) ########## superset-frontend/src/explore/components/controls/TimeRangeControl/index.tsx: ########## @@ -0,0 +1,47 @@ +import { useMemo } from 'react'; +import dayjs from 'dayjs'; +import { TimeRangePicker } from 'src/components/TimePicker'; +import ControlHeader, { ControlHeaderProps } from '../../ControlHeader'; + +type TimeRangeValueType = [string, string]; + +export interface TimeRangeControlProps extends ControlHeaderProps { + value?: TimeRangeValueType; + onChange?: (value: TimeRangeValueType, errors: any) => void; + allowClear?: boolean; + showNow?: boolean; + allowEmpty?: [boolean, boolean]; +} + +export default function TimeRangeControl({ + value: stringValue, + onChange, + allowClear, + showNow, + allowEmpty, + ...rest +}: TimeRangeControlProps) { + const dayjsValue = useMemo(() => { + const ret: [dayjs.Dayjs | null, dayjs.Dayjs | null] = [null, null]; + if (stringValue?.[0]) { + ret[0] = dayjs.utc(stringValue[0], 'HH:mm:ss'); Review Comment: ### Missing error handling for date parsing <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The dayjs.utc() parsing operation could fail with invalid time strings, but there's no error handling for this case. ###### Why this matters If the input string is not in the expected 'HH:mm:ss' format, the application might silently fail or produce incorrect dates without any indication of the parsing error. ###### Suggested change ∙ *Feature Preview* Add try-catch block around the date parsing operations and propagate errors through the onChange callback: ```typescript const dayjsValue = useMemo(() => { const ret: [dayjs.Dayjs | null, dayjs.Dayjs | null] = [null, null]; let error = null; try { if (stringValue?.[0]) { ret[0] = dayjs.utc(stringValue[0], 'HH:mm:ss'); if (!ret[0].isValid()) throw new Error('Invalid start time format'); } if (stringValue?.[1]) { ret[1] = dayjs.utc(stringValue[1], 'HH:mm:ss'); if (!ret[1].isValid()) throw new Error('Invalid end time format'); } } catch (e) { error = e instanceof Error ? e.message : 'Invalid time format'; } return { value: ret, error }; }, [stringValue]); ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ebe569a8-5882-4f5b-a540-76b3ebf891eb/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ebe569a8-5882-4f5b-a540-76b3ebf891eb?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ebe569a8-5882-4f5b-a540-76b3ebf891eb?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ebe569a8-5882-4f5b-a540-76b3ebf891eb?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ebe569a8-5882-4f5b-a540-76b3ebf891eb) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:00f166b0-bece-4a03-8698-04cc7c81e5e8 --> [](00f166b0-bece-4a03-8698-04cc7c81e5e8) ########## superset-frontend/plugins/plugin-chart-echarts/src/Timeline/index.ts: ########## @@ -0,0 +1,36 @@ +import { Behavior, t } from '@superset-ui/core'; +import transformProps from './transformProps'; +import controlPanel from './controlPanel'; +import buildQuery from './buildQuery'; +import { EchartsChartPlugin } from '../types'; +import thumbnail from './images/thumbnail.png'; +import example1 from './images/example1.png'; +import example2 from './images/example2.png'; + +export default class EchartsTimelineChartPlugin extends EchartsChartPlugin { + constructor() { + super({ + buildQuery, + controlPanel, + loadChart: () => import('./EchartsTimeline'), Review Comment: ### Unhandled Dynamic Import <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Dynamic import without error handling or loading state management could lead to uncaught promise rejections and poor user experience during chart loading failures. ###### Why this matters If the dynamic import fails, it could cause the entire chart to fail silently or provide no feedback to the user, impacting the application's reliability and user experience. ###### Suggested change ∙ *Feature Preview* Implement error handling and loading state management for the dynamic import. Example: ```typescript loadChart: () => import('./EchartsTimeline').catch(error => { console.error('Failed to load Timeline chart:', error); throw error; }) ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c508f9f7-6c61-49e4-b757-fb64eb937719/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c508f9f7-6c61-49e4-b757-fb64eb937719?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c508f9f7-6c61-49e4-b757-fb64eb937719?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c508f9f7-6c61-49e4-b757-fb64eb937719?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c508f9f7-6c61-49e4-b757-fb64eb937719) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:fb7ab3b5-118d-4f09-98a9-810e483b2e9a --> [](fb7ab3b5-118d-4f09-98a9-810e483b2e9a) ########## superset-frontend/plugins/plugin-chart-echarts/src/Timeline/constants.ts: ########## @@ -0,0 +1,8 @@ +export const ELEMENT_HEIGHT_SCALE = 0.85 as const; + +export enum Dimension { + StartTime = 'start_time', + EndTime = 'end_time', + Index = 'index', + SeriesCount = 'series_count', +} Review Comment: ### Inconsistent Enum Naming Convention <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The enum naming is inconsistent, mixing camelCase for enum members with snake_case for string values. ###### Why this matters Inconsistent naming patterns increase cognitive load and can lead to confusion when using the enum throughout the codebase. ###### Suggested change ∙ *Feature Preview* ```typescript export enum Dimension { StartTime = 'startTime', EndTime = 'endTime', Index = 'index', SeriesCount = 'seriesCount', } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4a26bb6f-acc9-4ec1-aef6-200874953369/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4a26bb6f-acc9-4ec1-aef6-200874953369?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4a26bb6f-acc9-4ec1-aef6-200874953369?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4a26bb6f-acc9-4ec1-aef6-200874953369?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4a26bb6f-acc9-4ec1-aef6-200874953369) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:a9fd45b5-0dff-4bb2-9393-bf4529215ac9 --> [](a9fd45b5-0dff-4bb2-9393-bf4529215ac9) ########## superset-frontend/plugins/plugin-chart-echarts/src/Timeline/index.ts: ########## @@ -0,0 +1,36 @@ +import { Behavior, t } from '@superset-ui/core'; +import transformProps from './transformProps'; +import controlPanel from './controlPanel'; +import buildQuery from './buildQuery'; +import { EchartsChartPlugin } from '../types'; +import thumbnail from './images/thumbnail.png'; +import example1 from './images/example1.png'; +import example2 from './images/example2.png'; + +export default class EchartsTimelineChartPlugin extends EchartsChartPlugin { + constructor() { + super({ + buildQuery, + controlPanel, + loadChart: () => import('./EchartsTimeline'), + metadata: { + behaviors: [ + Behavior.InteractiveChart, + Behavior.DrillToDetail, + Behavior.DrillBy, + ], + credits: ['https://echarts.apache.org'], + name: t('Timeline'), + description: t( + 'Timeline chart visualizes important events over a time span. ' + + 'Every data point displayed as a separate event along a ' + + 'horizontal line.', + ), Review Comment: ### Use template literals for multi-line strings <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? String concatenation using + operator for a multi-line description makes the text harder to read and maintain. ###### Why this matters String concatenation with + operator creates visual noise and makes text modifications more error-prone. Template literals provide better readability for multi-line strings. ###### Suggested change ∙ *Feature Preview* ```typescript description: t(` Timeline chart visualizes important events over a time span. Every data point displayed as a separate event along a horizontal line. `), ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4ff1a3c3-f129-4ec5-99e5-6c3a5787ef3a/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4ff1a3c3-f129-4ec5-99e5-6c3a5787ef3a?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4ff1a3c3-f129-4ec5-99e5-6c3a5787ef3a?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4ff1a3c3-f129-4ec5-99e5-6c3a5787ef3a?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4ff1a3c3-f129-4ec5-99e5-6c3a5787ef3a) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:40c0321a-9fd9-4f0d-a00c-559330476bf7 --> [](40c0321a-9fd9-4f0d-a00c-559330476bf7) ########## superset-frontend/src/explore/components/controls/TimeRangeControl/index.tsx: ########## @@ -0,0 +1,47 @@ +import { useMemo } from 'react'; +import dayjs from 'dayjs'; +import { TimeRangePicker } from 'src/components/TimePicker'; +import ControlHeader, { ControlHeaderProps } from '../../ControlHeader'; + +type TimeRangeValueType = [string, string]; + +export interface TimeRangeControlProps extends ControlHeaderProps { + value?: TimeRangeValueType; + onChange?: (value: TimeRangeValueType, errors: any) => void; + allowClear?: boolean; + showNow?: boolean; + allowEmpty?: [boolean, boolean]; +} Review Comment: ### Missing interface prop descriptions <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The interface props lack descriptions explaining their purpose and expected values, particularly for non-obvious props like allowEmpty which uses a tuple type. ###### Why this matters Without clear prop descriptions, other developers will need to investigate the implementation or experiment to understand the expected behavior of each prop. ###### Suggested change ∙ *Feature Preview* /** * Props for the TimeRangeControl component */ export interface TimeRangeControlProps extends ControlHeaderProps { /** The current time range value in HH:mm:ss format */ value?: TimeRangeValueType; /** Callback when time range changes. Errors are currently unused */ onChange?: (value: TimeRangeValueType, errors: any) => void; /** Whether to show the clear button */ allowClear?: boolean; /** Whether to show the 'Now' shortcut */ showNow?: boolean; /** Controls whether start and end times can be empty [startEmpty, endEmpty] */ allowEmpty?: [boolean, boolean]; } ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9c112b53-eb04-40fb-949f-e4d65d395b87/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9c112b53-eb04-40fb-949f-e4d65d395b87?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9c112b53-eb04-40fb-949f-e4d65d395b87?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9c112b53-eb04-40fb-949f-e4d65d395b87?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9c112b53-eb04-40fb-949f-e4d65d395b87) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:ed7a079f-a7f6-45af-b608-0ac73b02eaf7 --> [](ed7a079f-a7f6-45af-b608-0ac73b02eaf7) ########## superset-frontend/src/explore/components/controls/TimeRangeControl/index.tsx: ########## @@ -0,0 +1,47 @@ +import { useMemo } from 'react'; +import dayjs from 'dayjs'; +import { TimeRangePicker } from 'src/components/TimePicker'; +import ControlHeader, { ControlHeaderProps } from '../../ControlHeader'; + +type TimeRangeValueType = [string, string]; + +export interface TimeRangeControlProps extends ControlHeaderProps { + value?: TimeRangeValueType; + onChange?: (value: TimeRangeValueType, errors: any) => void; + allowClear?: boolean; + showNow?: boolean; + allowEmpty?: [boolean, boolean]; +} + +export default function TimeRangeControl({ + value: stringValue, + onChange, + allowClear, + showNow, + allowEmpty, + ...rest +}: TimeRangeControlProps) { + const dayjsValue = useMemo(() => { + const ret: [dayjs.Dayjs | null, dayjs.Dayjs | null] = [null, null]; + if (stringValue?.[0]) { + ret[0] = dayjs.utc(stringValue[0], 'HH:mm:ss'); + } + if (stringValue?.[1]) { + ret[1] = dayjs.utc(stringValue[1], 'HH:mm:ss'); + } + return ret; + }, [stringValue]); Review Comment: ### Unnecessary Memoization <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The useMemo hook is used for a relatively simple time parsing operation that doesn't justify the overhead of memoization. ###### Why this matters Memoization adds memory overhead and complexity to maintain the cache. For simple operations like parsing two time strings, the performance benefit is negligible and the overhead of memoization might actually be more costly. ###### Suggested change ∙ *Feature Preview* Remove the useMemo hook and perform the time parsing directly: ```typescript const dayjsValue: [dayjs.Dayjs | null, dayjs.Dayjs | null] = [ stringValue?.[0] ? dayjs.utc(stringValue[0], 'HH:mm:ss') : null, stringValue?.[1] ? dayjs.utc(stringValue[1], 'HH:mm:ss') : null ]; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/97d6e987-9939-44ad-81ed-8313ffcb2534/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/97d6e987-9939-44ad-81ed-8313ffcb2534?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/97d6e987-9939-44ad-81ed-8313ffcb2534?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/97d6e987-9939-44ad-81ed-8313ffcb2534?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/97d6e987-9939-44ad-81ed-8313ffcb2534) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:8766a5bf-5b39-41c8-baa2-bc156b4c208a --> [](8766a5bf-5b39-41c8-baa2-bc156b4c208a) ########## superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/SmoothLine/controlPanel.tsx: ########## @@ -99,18 +98,7 @@ const config: ControlPanelConfig = { }, }, ], - [ - { - name: 'zoomable', - config: { - type: 'CheckboxControl', - label: t('Data Zoom'), - default: zoomable, - renderTrigger: true, - description: t('Enable data zooming controls'), - }, - }, - ], + ['zoomable'], Review Comment: ### Potentially Missing Zoom Control Definition <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The zoomable control is simplified to a string reference but there's no indication that this control exists elsewhere in the codebase. This could lead to a missing or non-functional zoom control in the UI. ###### Why this matters If the zoomable control is not properly defined elsewhere, users won't be able to enable/disable data zooming functionality in the chart, which is a critical feature for time series visualizations. ###### Suggested change ∙ *Feature Preview* Either define the zoomable control in a shared controls file or restore the original implementation: ```typescript [ { name: 'zoomable', config: { type: 'CheckboxControl', label: t('Data Zoom'), default: zoomable, renderTrigger: true, description: t('Enable data zooming controls'), }, }, ] ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a60cfd32-b303-4bac-aa96-f139bf757105/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a60cfd32-b303-4bac-aa96-f139bf757105?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a60cfd32-b303-4bac-aa96-f139bf757105?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a60cfd32-b303-4bac-aa96-f139bf757105?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a60cfd32-b303-4bac-aa96-f139bf757105) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:997a7253-51a3-4122-9952-78d7a7dd708a --> [](997a7253-51a3-4122-9952-78d7a7dd708a) -- 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-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
