Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

feat: properly translate the table chart #724

Merged
merged 5 commits into from
Aug 5, 2020

Conversation

ktmud
Copy link
Contributor

@ktmud ktmud commented Aug 4, 2020

🏆 Enhancements

Enhance the superset-ui/translation package to support adding client side translations on demand, paving the road for dynamic plugin loading.

Using the table chart as an example.

cc @suddjian @rusackas @kristw @stuarthu

@ktmud ktmud requested a review from a team as a code owner August 4, 2020 07:27
@vercel
Copy link

vercel bot commented Aug 4, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/4wuaoxiie
✅ Preview: https://superset-ui-git-fork-ktmud-table-translation.superset.vercel.app

Copy link
Contributor

@stuarthu stuarthu left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the quick fix

@codecov
Copy link

codecov bot commented Aug 4, 2020

Codecov Report

Merging #724 into master will increase coverage by 0.20%.
The diff coverage is 73.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #724      +/-   ##
==========================================
+ Coverage   24.14%   24.35%   +0.20%     
==========================================
  Files         338      340       +2     
  Lines        7607     7636      +29     
  Branches      921      929       +8     
==========================================
+ Hits         1837     1860      +23     
- Misses       5699     5703       +4     
- Partials       71       73       +2     
Impacted Files Coverage Δ
...s/superset-ui-translation/test/languagePacks/en.ts 100.00% <ø> (ø)
...s/superset-ui-translation/test/languagePacks/zh.ts 100.00% <ø> (ø)
...ins/plugin-chart-table/src/DataTable/DataTable.tsx 46.87% <0.00%> (-0.75%) ⬇️
...-table/src/DataTable/components/SelectPageSize.tsx 40.00% <16.66%> (-21.54%) ⬇️
plugins/plugin-chart-table/src/TableChart.tsx 57.14% <57.14%> (+1.44%) ⬆️
packages/superset-ui-core/src/utils/logging.ts 100.00% <100.00%> (ø)
packages/superset-ui-translation/src/Translator.ts 100.00% <100.00%> (ø)
...superset-ui-translation/src/TranslatorSingleton.ts 100.00% <100.00%> (ø)
plugins/plugin-chart-table/src/controlPanel.tsx 37.03% <100.00%> (+2.42%) ⬆️
plugins/plugin-chart-table/src/i18n.ts 100.00% <100.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9455f89...71d7909. Read the comment docs.

.translate(key)
.ifPlural(plural, key)
.fetch(plural, num, ...args);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allow tn('key', 2) so we don't have to provide the key twice (t('key', 'key', 2)).

@rusackas
Copy link
Member

rusackas commented Aug 4, 2020

This is AWESOME. Thank you!!! Will review in detail asap.


export const PAGE_SIZE_OPTIONS = formatSelectOptions<number>([[0, t('All')], 10, 20, 50, 100, 200]);
addLocaleData(i18n);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to inject the locale data here because it was used by the controlPanel module before it can be imported. Ideally this should be in a more stable place, e.g., Plugin.setup(...) or Plugin.contructor(...), but that requires refactoring controlPanel to be loaded async. Let's leave it to a future PR.

@vercel vercel bot temporarily deployed to Preview August 5, 2020 01:49 Inactive
@ktmud ktmud force-pushed the table-translation branch from 871d6ab to 89bb55f Compare August 5, 2020 01:52
Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

One potential/hypothetical future improvement (just for discussion) might be to namespace translations (by plugin key, for example). In this (awesome!) PR, it appears that the first plugin to register a translation string wins. Which is a huge improvement! But I could see some theoretical case where two or more plugins attempt to provide translations for the same string. Those translations might be better or worse than one another, or have some different context. Having them prefer their own translations and fall back to others might prevent some headaches. Anyway, we can bolt that on later, if the situation ever arises.

This is great work. Thanks for tackling it!

@ktmud
Copy link
Contributor Author

ktmud commented Aug 5, 2020

LGTM! 🎉

One potential/hypothetical future improvement (just for discussion) might be to namespace translations (by plugin key, for example). In this (awesome!) PR, it appears that the first plugin to register a translation string wins. Which is a huge improvement! But I could see some theoretical case where two or more plugins attempt to provide translations for the same string. Those translations might be better or worse than one another, or have some different context. Having them prefer their own translations and fall back to others might prevent some headaches. Anyway, we can bolt that on later, if the situation ever arises.

This is great work. Thanks for tackling it!

Thanks! I agree we should probably have some kind of namespacing and preferably even enable that for other parts of Superset, too. But before we tackle i18n more seriously, I don't feel like spending too much time on it.

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

Successfully merging this pull request may close these issues.

3 participants