korbit-ai[bot] commented on code in PR #32707:
URL: https://github.com/apache/superset/pull/32707#discussion_r1999520297
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -420,6 +422,16 @@ export default function transformProps(
}
});
+ if(stackbydimension && stackdimension
+ && chartProps.rawFormData.groupby){
+ const idxSelectedDimension = formData.metrics.length > 1 ? 1 : 0
+ + chartProps.rawFormData.groupby.indexOf(stackdimension);
Review Comment:
### Incorrect Operator Precedence in Dimension Index Calculation
<sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The operator precedence in the expression is incorrect. The addition will be
performed before the ternary operation, leading to incorrect dimension index
calculation.
###### Why this matters
This will cause incorrect stack grouping as the wrong dimension index will
be used, potentially stacking data points that shouldn't be stacked together.
###### Suggested change ∙ *Feature Preview*
Wrap the addition in parentheses to ensure correct operator precedence:
```typescript
const idxSelectedDimension = formData.metrics.length > 1 ? 1 : (0 +
chartProps.rawFormData.groupby.indexOf(stackdimension));
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c1decd5d-1fe0-4406-b3fd-793276058a71?suggestedFixEnabled=true)
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:1bd124d4-3d7c-4799-9ab5-7944f45130a5 -->
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx:
##########
@@ -317,6 +319,42 @@
label: t('Chart Options'),
expanded: true,
controlSetRows: [
+ [
+ {
+ name: 'stackbydimension',
+ config: {
+ type: 'CheckboxControl',
+ label: t('Stack by dimension'),
+ default: stackbydimension,
+ renderTrigger: true,
+ description: t('Stack in groups, where each group corresponds to
a dimension'),
+ },
+ },
+ ],
+ [
+ {
+ name: 'stackdimension',
+ config: {
+ type: 'SelectControl',
+ label: t('Dimension to stack by'),
+ renderTrigger: true,
+ description: t('Stack in groups, where each group corresponds to
a dimension'),
+ shouldMapStateToProps: (prevState, state, controlState,
chartState) => {
+ return true;
+ },
Review Comment:
### Redundant shouldMapStateToProps Function <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The shouldMapStateToProps function always returns true and serves no
purpose, making the code unnecessarily complex.
###### Why this matters
This adds cognitive overhead as developers need to understand why this
function exists when it doesn't provide any conditional logic.
###### Suggested change ∙ *Feature Preview*
Remove the shouldMapStateToProps property as it's not providing any value:
```typescript
config: {
type: 'SelectControl',
label: t('Dimension to stack by'),
renderTrigger: true,
description: t('Stack in groups, where each group corresponds to a
dimension'),
mapStateToProps: (state, controlState, chartState) => {
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d1e852cf-c343-4097-a4c9-14390cb99d4d?suggestedFixEnabled=true)
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:2612b9af-35e0-405f-aa39-ddc2c28b5772 -->
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx:
##########
@@ -317,6 +319,42 @@ const config: ControlPanelConfig = {
label: t('Chart Options'),
expanded: true,
controlSetRows: [
+ [
+ {
+ name: 'stackbydimension',
+ config: {
+ type: 'CheckboxControl',
+ label: t('Stack by dimension'),
+ default: stackbydimension,
+ renderTrigger: true,
+ description: t('Stack in groups, where each group corresponds to
a dimension'),
+ },
+ },
+ ],
+ [
+ {
+ name: 'stackdimension',
+ config: {
+ type: 'SelectControl',
+ label: t('Dimension to stack by'),
Review Comment:
### Missing conditional visibility for stack dimension control
<sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The stackdimension control is not conditionally visible based on the
stackbydimension checkbox state.
###### Why this matters
Users will see and potentially interact with the stackdimension dropdown
even when stacking by dimension is disabled, leading to confusion and potential
misconfigurations.
###### Suggested change ∙ *Feature Preview*
Add visibility configuration to the control:
```typescript
name: 'stackdimension',
config: {
type: 'SelectControl',
label: t('Dimension to stack by'),
visibility: ({ controls }) => Boolean(controls?.stackbydimension?.value),
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e2e45e12-4303-4ce0-8666-c4afbda7dc47?suggestedFixEnabled=true)
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ed7ff76a-39dc-4219-97fb-db4ae0ca0b2d -->
--
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
