codeant-ai-for-open-source[bot] commented on code in PR #38695:
URL: https://github.com/apache/superset/pull/38695#discussion_r2960736688
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx:
##########
@@ -201,6 +201,30 @@ function createCustomizeSection(
},
},
],
+ [
+ {
+ name: `label_position${controlSuffix}`,
+ config: {
+ type: 'SelectControl',
+ freeForm: false,
+ label: t('Label Position'),
+ choices: [
+ ['top', t('Top')],
+ ['inside', t('Inside')],
+ ['bottom', t('Bottom')],
+ ['left', t('Left')],
+ ['right', t('Right')],
+ ],
+ default: 'top',
+ renderTrigger: true,
Review Comment:
**Suggestion:** The new label-position control omits the `auto` option and
hardcodes the default to `top`, which breaks the expected contract used
elsewhere (`labelPosition` defaults to `auto`) and prevents users from
restoring orientation-aware behavior. Add `auto` to the choices and make it the
default so mixed timeseries stays consistent with existing chart defaults and
transform logic. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Mixed chart UI deviates from shared label contract.
- ⚠️ Users cannot reselect auto label behavior.
- ⚠️ Form-data defaults inconsistent across ECharts timeseries charts.
```
</details>
```suggestion
['auto', t('Auto')],
['top', t('Top')],
['inside', t('Inside')],
['bottom', t('Bottom')],
['left', t('Left')],
['right', t('Right')],
],
default: 'auto',
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open Mixed Chart Explore UI (plugin wired via
`MixedTimeseries/index.ts:49-52` using
`controlPanel` and `transformProps`).
2. In Customize section (`MixedTimeseries/controlPanel.tsx:206-229`), enable
`show_value`
then inspect `label_position`: options are only top/inside/bottom/left/right
and default
is hardcoded `'top'`.
3. Compare shared ECharts label control (`controls.tsx:143-29`) and
timeseries defaults
(`Timeseries/constants.ts:89`): both include `'auto'` and default to
`'auto'`.
4. Render path uses `MixedTimeseries/transformProps.ts:228` (merges
defaults/formData)
then passes `labelPosition` to `transformSeries`
(`transformProps.ts:483,558`), where
`'auto'` has explicit fallback behavior
(`Timeseries/transformers.ts:416-421`). With
current control, users cannot select/restore `'auto'` from UI, creating an
inconsistent
contract.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx
**Line:** 212:219
**Comment:**
*Logic Error: The new label-position control omits the `auto` option
and hardcodes the default to `top`, which breaks the expected contract used
elsewhere (`labelPosition` defaults to `auto`) and prevents users from
restoring orientation-aware behavior. Add `auto` to the choices and make it the
default so mixed timeseries stays consistent with existing chart defaults and
transform logic.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38695&comment_hash=f77b6f3930153fa7f56d90717f6b6d91b8fbe406be1cd9b95bbef6190a493c9c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38695&comment_hash=f77b6f3930153fa7f56d90717f6b6d91b8fbe406be1cd9b95bbef6190a493c9c&reaction=dislike'>👎</a>
--
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]
