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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

aehanno
Copy link
Contributor

@aehanno aehanno commented May 16, 2022

SUMMARY

translating variables

ADDITIONAL INFORMATION

Fixes #20060

@aehanno
Copy link
Contributor Author

aehanno commented Jun 6, 2022

@villebro , i will be glad to discuss with you about variable translation.

Without adding translation on those varaibles, we have any translation on the second part of those sentence
I only find this solution and after testing, I didn't catch error with this correction

I understand your worries about that, and i will be glad to understand any other solution to solve that problem

@villebro
Copy link
Member

@aehanno can you show a before and after of these changes? I'd like to understand how these translations are being picked up

@codecov
Copy link

codecov bot commented Jun 15, 2022

Codecov Report

Merging #20080 (afb2301) into master (17fbb2d) will decrease coverage by 11.04%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master   #20080       +/-   ##
===========================================
- Coverage   67.47%   56.44%   -11.04%     
===========================================
  Files        1880     1880               
  Lines       72283    72282        -1     
  Branches     7881     7880        -1     
===========================================
- Hits        48772    40797     -7975     
- Misses      21486    29460     +7974     
  Partials     2025     2025               
Flag Coverage Δ
hive 52.73% <ø> (ø)
mysql ?
postgres ?
presto 52.62% <ø> (ø)
python 59.18% <ø> (-22.95%) ⬇️
sqlite ?
unit 52.46% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ivot-table/src/react-pivottable/TableRenderers.jsx 0.00% <ø> (ø)
.../components/FiltersBadge/FilterIndicator/index.tsx 87.50% <ø> (ø)
...t-frontend/src/dashboard/components/SliceAdder.jsx 60.27% <ø> (ø)
...nts/controls/DateFilterControl/DateFilterLabel.tsx 61.45% <ø> (-0.40%) ⬇️
superset/utils/dashboard_import_export.py 0.00% <0.00%> (-100.00%) ⬇️
superset/tags/core.py 4.54% <0.00%> (-95.46%) ⬇️
superset/key_value/commands/update.py 0.00% <0.00%> (-90.91%) ⬇️
superset/key_value/commands/delete.py 0.00% <0.00%> (-87.88%) ⬇️
superset/key_value/commands/delete_expired.py 0.00% <0.00%> (-84.00%) ⬇️
superset/dashboards/commands/importers/v0.py 15.62% <0.00%> (-76.25%) ⬇️
... and 288 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@aehanno
Copy link
Contributor Author

aehanno commented Jun 15, 2022

@aehanno can you show a before and after of these changes? I'd like to understand how these translations are being picked up

Hey @villebro, yes of course, I can show you an exemple.
It's in dashboard edition in charts tab.

For this one, I added modification in SliceAdder.jsx file,

There is before my correction label: t('Sort by %s', label),
Before

And this one is after my correction label: t('Sort by %s', t(label)),
After

We can see the second part is well translate in the second image

@aehanno aehanno changed the title fix : translating variables fix: translating variables Jun 15, 2022
@@ -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.

@@ -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?

"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?

@rusackas
Copy link
Member

Looks like this needs a rebase. There have been some other PRs recently adding translations as well, so I hope it's not too painful.

We also still need some validation of the language itself, if @mistercrunch or anyone else that speaks French has time to scope it out :)

@rusackas
Copy link
Member

rusackas commented Feb 9, 2024

Sadly, it looks like this never passed, CI, never got validation of the translated strings, and now is in need of a rebase again. @aehanno would you mind doing a rebase, and we can try again to get this across the finish line? Thanks!

"No matching records found": ["Aucun résultat trouvé"]
"No matching records found": ["Aucun résultat trouvé"],
"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"

@@ -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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing French Translation
4 participants