bito-code-review[bot] commented on code in PR #44657:
URL: https://github.com/apache/superset/pull/44657#discussion_r4176192207
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx:
##########
@@ -173,7 +177,10 @@ const config: ControlPanelConfig = {
name: 'rowTotals',
config: {
type: 'CheckboxControl',
- label: t('Show rows total'),
+ // The displayed value may be a result aggregation (Median,
+ // Average, ...) rather than a plain total once
`aggregateFunction`
+ // is set below, so "summary" rather than "total".
+ label: () => t('Show row summaries'),
Review Comment:
<!-- Bito Reply -->
The suggestion to use a static label is appropriate because the label does
not depend on the control's state, which aligns with the consistency standards
used for other controls in the file. Applying this change simplifies the code
by removing the unnecessary function-form indirection.
**superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx**
```
type: 'CheckboxControl',
label: t('Show row summaries'),
```
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx:
##########
@@ -197,7 +204,7 @@ const config: ControlPanelConfig = {
name: 'colTotals',
config: {
type: 'CheckboxControl',
- label: t('Show columns total'),
+ label: () => t('Show column summaries'),
Review Comment:
<!-- Bito Reply -->
The suggestion to use a static label is appropriate here. It aligns the code
with the established standard in the file, where other similar controls use
static labels, and improves consistency across the panel.
**superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx**
```
type: 'CheckboxControl',
- label: t('Show columns total'),
+ label: t('Show column summaries'),
```
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx:
##########
@@ -243,6 +250,34 @@ const config: ControlPanelConfig = {
},
},
],
+ [
+ {
+ name: 'aggregateFunction',
+ config: {
+ type: 'SelectControl',
+ label: () => t('Aggregation function'),
Review Comment:
<!-- Bito Reply -->
The suggestion to use a static label is appropriate. Replacing the function
form with a static string improves consistency with the existing controls in
the file and aligns with the project's development standards.
**superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx**
```
config: {
type: 'SelectControl',
label: t('Aggregation function'),
```
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx:
##########
@@ -243,6 +250,34 @@ const config: ControlPanelConfig = {
},
},
],
+ [
+ {
+ name: 'aggregateFunction',
+ config: {
+ type: 'SelectControl',
+ label: () => t('Aggregation function'),
+ default: 'Metric',
Review Comment:
<!-- Bito Reply -->
Exporting the 'Metric' sentinel to a shared constant is a good practice. It
ensures consistency across the default value, choice options, and visibility
logic, preventing potential bugs where these values might otherwise diverge.
--
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]
