bito-code-review[bot] commented on code in PR #44657:
URL: https://github.com/apache/superset/pull/44657#discussion_r4176783592
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx:
##########
@@ -197,7 +205,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 is appropriate because it addresses a known issue where eager
evaluation of `t()` in configuration objects prevents dynamic language
switching. Applying this change ensures the label is evaluated at runtime,
which is the correct pattern for this codebase.
**superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx**
```
label: () => t('Show column summaries'),
```
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx:
##########
@@ -173,7 +178,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 reviewer's suggestion to use `label: () => t(...)` is intended to avoid
capturing the fallback language at module load time, which is a known best
practice for configuration files in this repository. While the user correctly
notes that the existing file uses eager `t(...)` labels throughout, adopting
the suggested functional form is a proactive improvement that aligns with the
repository's `i18n-strings/no-eager-t-in-config` linting rule and ensures
correct behavior during runtime language switches.
**superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx**
```
label: () => t('Show row summaries'),
```
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx:
##########
@@ -243,6 +251,34 @@ const config: ControlPanelConfig = {
},
},
],
+ [
+ {
+ name: 'aggregateFunction',
+ config: {
+ type: 'SelectControl',
+ label: t('Aggregation function'),
Review Comment:
<!-- Bito Reply -->
The suggestion to use an arrow function `() => t(...)` for the `label`
property is correct and aligns with the `i18n-strings/no-eager-t-in-config`
rule. This ensures that the translation is evaluated at runtime when the
language changes, rather than being fixed at module load time. Applying this
change is recommended to maintain consistency and proper i18n behavior.
**superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx**
```
label: () => t('Aggregation function'),
```
--
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]
