Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: translating variables #20080

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import React from 'react';
import PropTypes from 'prop-types';
import { t } from '@superset-ui/core';
import { PivotData, flatKey } from './utilities';
import { Styles } from './Styles';

Expand Down Expand Up @@ -470,6 +471,8 @@ export class TableRenderer extends React.Component {
i += colSpan;
}

const constAggregatorName = `${this.props.aggregatorName}`;

const totalCell =
attrIdx === 0 && rowTotals ? (
<th
Expand All @@ -486,7 +489,7 @@ export class TableRenderer extends React.Component {
true,
)}
>
{`Total (${this.props.aggregatorName})`}
{t(`Total (%s)`, t(constAggregatorName))}
</th>
) : null;

Expand All @@ -498,6 +501,8 @@ export class TableRenderer extends React.Component {
// Render just the attribute names of the rows (the actual attribute values
// will show up in the individual rows).

const constAggregatorName = `${this.props.aggregatorName}`;

const {
rowAttrs,
colAttrs,
Expand Down Expand Up @@ -535,6 +540,7 @@ export class TableRenderer extends React.Component {
</th>
);
})}

<th
className="pvtTotalLabel"
key="padding"
Expand All @@ -549,7 +555,7 @@ export class TableRenderer extends React.Component {
)}
>
{colAttrs.length === 0
? `Total (${this.props.aggregatorName})`
? t(`Total (%s)`, t(constAggregatorName))
: null}
</th>
</tr>
Expand Down Expand Up @@ -748,6 +754,8 @@ export class TableRenderer extends React.Component {
grandTotalCallback,
} = pivotSettings;

const constAggregatorName = `${this.props.aggregatorName}`;

const totalLabelCell = (
<th
key="label"
Expand All @@ -763,7 +771,7 @@ export class TableRenderer extends React.Component {
true,
)}
>
{`Total (${this.props.aggregatorName})`}
{t(`Total (%s)`, t(constAggregatorName))}
</th>
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import { SearchOutlined } from '@ant-design/icons';
import React, { FC } from 'react';
import { t } from '@superset-ui/core';
import { getFilterValueForDisplay } from 'src/dashboard/components/nativeFilters/FilterBar/FilterSets/utils';
import {
FilterIndicatorText,
Expand Down Expand Up @@ -51,7 +52,7 @@ const FilterIndicator: FC<IndicatorProps> = ({
{name}
{resultValue ? ': ' : ''}
</Title>
<FilterValue>{resultValue}</FilterValue>
<FilterValue>{t(resultValue)}</FilterValue>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like something that might not have a static/expected value, is that correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey in my understanding of the translation framework, it does static analysis of the code to find translatable strings, it only works on constants not on variables.

</Item>
{text && <FilterIndicatorText>{text}</FilterIndicatorText>}
</>
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/dashboard/components/SliceAdder.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ class SliceAdder extends React.Component {
value={this.state.sortBy}
onChange={this.handleSelect}
options={Object.entries(KEYS_TO_SORT).map(([key, label]) => ({
label: t('Sort by %s', label),
label: t('Sort by %s', t(label)),
value: key,
}))}
placeholder={t('Sort by')}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ export default function DateFilterLabel(props: DateFilterControlProps) {
<Divider />
<div>
<div className="section-title">{t('Actual time range')}</div>
{validTimeRange && <div>{evalResponse}</div>}
{validTimeRange && <div>{t(evalResponse)}</div>}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here... is this something we would expect to have in the translation files, or is it a more dynamic value (as it sounds, based on the name)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's a date or it's write 'No filter', so I think in fact that will juste translate 'no filter' and don't touch to other result

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's worth putting a little ternary condition or similar in there, e.g. evalResponse = 'no filter' ? t('no filter') : evalResponse? Or maybe there's somewhere further upstream in the code that's more appropriate to the the translation?

{!validTimeRange && (
<IconWrapper className="warning">
<Icons.ErrorSolidSmall iconColor={theme.colors.error.base} />
Expand Down Expand Up @@ -373,7 +373,7 @@ export default function DateFilterLabel(props: DateFilterControlProps) {
>
<Tooltip placement="top" title={tooltipTitle}>
<Label className="pointer" data-test="time-range-trigger">
{actualTimeRange}
{t(actualTimeRange)}
</Label>
</Tooltip>
</StyledPopover>
Expand Down
22 changes: 21 additions & 1 deletion superset/translations/fr/LC_MESSAGES/messages.json
Original file line number Diff line number Diff line change
Expand Up @@ -3877,7 +3877,27 @@
],
"Scope": ["Périmètre"],
"Dependent on": ["Dépend de"],
"No matching records found": ["Aucun résultat trouvé"]
"No matching records found": ["Aucun résultat trouvé"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't the .json file generated here? shouldn't this be in the .po file?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes indeed! I thought that was covered in this PR, but that doesn't seem to be the case.

"Trend": ["Tendance"],
"Total (Count)": ["Total (Somme)"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Count != Somme
more like "Compte" ou "Nombre"

"Total (%s)": ["Total (%s)"],
"Total (Count Unique Values)": ["Total (Somme)"],
"Total (List Unique Values)": ["Total (Somme)"],
"Sum": ["Somme"],
"Average": ["Moyenne"],
"Median": ["Médiane"],
"Sample Variance": ["Variance échantillon"],
"Sample Standard Deviation": ["Écart-type échantillon"],
"Minimum": ["Minimum"],
"Maximum": ["Maximum"],
"First": ["Premier"],
"Last": ["Dernier"],
"Sum as Fraction of Total": ["Somme comme fraction du total"],
"Sum as Fraction of Rows": ["Somme comme fraction des lignes"],
"Sum as Fraction of Columns": ["Somme comme fraction des colonnes"],
"Count as Fraction of Total": ["Décompte comme fraction du total"],
"Count as Fraction of Rows": ["Décompte comme fraction des lignes"],
"Count as Fraction of Columns": ["Décompte comme fraction des colonnes"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mistercrunch do the above translations look correct to you?

}
}
}