korbit-ai[bot] commented on code in PR #35180:
URL: https://github.com/apache/superset/pull/35180#discussion_r2356903638
##########
superset-frontend/src/visualizations/TimeTable/components/Sparkline/Sparkline.tsx:
##########
@@ -55,6 +55,7 @@ const Sparkline = ({
yAxisBounds={yAxisBounds}
showYAxis={column.showYAxis || false}
entries={entries}
+ sparkType={column.sparkType || 'line'}
Review Comment:
### Missing sparkType validation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The sparkType property is being passed without type validation, potentially
allowing invalid spark types to be passed to SparklineCell component.
###### Why this matters
If column.sparkType contains an invalid value (not 'line', 'bar', or
'area'), it will be passed directly to SparklineCell, which could cause
rendering errors or unexpected behavior in the chart component.
###### Suggested change ∙ *Feature Preview*
Add type validation to ensure only valid spark types are passed:
```typescript
const validSparkTypes = ['line', 'bar', 'area'] as const;
type SparkType = typeof validSparkTypes[number];
const getValidSparkType = (sparkType: unknown): SparkType => {
return validSparkTypes.includes(sparkType as SparkType)
? (sparkType as SparkType)
: 'line';
};
// Then use:
sparkType={getValidSparkType(column.sparkType)}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e36a65ab-94a5-4219-b08c-5c71dd06cf7f/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e36a65ab-94a5-4219-b08c-5c71dd06cf7f?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e36a65ab-94a5-4219-b08c-5c71dd06cf7f?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e36a65ab-94a5-4219-b08c-5c71dd06cf7f?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e36a65ab-94a5-4219-b08c-5c71dd06cf7f)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:8fbca2b6-177b-4f32-b282-dd43e206fdb9 -->
[](8fbca2b6-177b-4f32-b282-dd43e206fdb9)
##########
superset-frontend/src/explore/components/controls/TimeSeriesColumnControl/index.jsx:
##########
@@ -229,6 +238,18 @@ export default class TimeSeriesColumnControl extends
Component {
/>,
)}
<Divider />
+ {this.state.colType === 'spark' &&
+ this.formRow(
+ t('Chart Type'),
+ t('Type of chart to display in sparkline'),
+ 'spark-type',
+ <Select
+ ariaLabel={t('Chart Type')}
+ value={this.state.sparkType || undefined}
+ onChange={this.onSelectChange.bind(this, 'sparkType')}
+ options={sparkTypeOptions}
+ />,
+ )}
Review Comment:
### Chart Type selector positioned after dimensions controls <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The Chart Type selector is positioned before the Width and Height controls,
but logically the chart type should be selected first before configuring
dimensions, as different chart types may have different optimal sizing
requirements.
###### Why this matters
Users may configure width and height before selecting the chart type,
potentially leading to suboptimal visual results when they later change the
chart type, requiring them to readjust dimensions.
###### Suggested change ∙ *Feature Preview*
Move the Chart Type selector to appear immediately after the main Type
selector and before the Width/Height controls:
```jsx
{this.state.colType === 'spark' &&
this.formRow(
t('Chart Type'),
t('Type of chart to display in sparkline'),
'spark-type',
<Select
ariaLabel={t('Chart Type')}
value={this.state.sparkType || undefined}
onChange={this.onSelectChange.bind(this, 'sparkType')}
options={sparkTypeOptions}
/>,
)}
<Divider />
{this.state.colType === 'spark' &&
this.formRow(
t('Width'),
// ... rest of width control
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f93752bb-40c3-45ab-ad09-02e9b51e3b35/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f93752bb-40c3-45ab-ad09-02e9b51e3b35?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f93752bb-40c3-45ab-ad09-02e9b51e3b35?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f93752bb-40c3-45ab-ad09-02e9b51e3b35?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f93752bb-40c3-45ab-ad09-02e9b51e3b35)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:7e6d2dba-04e8-4d26-8850-cdeff1690077 -->
[](7e6d2dba-04e8-4d26-8850-cdeff1690077)
--
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]
