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

add i18next and config for i18n of UI #2254

Merged
merged 17 commits into from
Dec 8, 2022
Merged

add i18next and config for i18n of UI #2254

merged 17 commits into from
Dec 8, 2022

Conversation

merobi-hub
Copy link
Collaborator

Signed-off-by: Michael Robinson [email protected]

Problem

Internationalization is strongly recommended by the LFAI (in the CII Silver Badge). The UI is not currently internationalized.

Closes: #ISSUE-NUMBER

Solution

This change adds i18next, configures it, and edits the text to make use of the tool's translation capabilities.

Checklist

  • You've signed-off your work
  • Your changes are accompanied by tests (if relevant)
  • Your change contains a small diff and is self-contained
  • You've updated any relevant documentation (if relevant)
  • You've updated the CHANGELOG.md with details about your change under the "Unreleased" section (if relevant, depending on the change, this may not be necessary)
  • You've versioned your .sql database schema migration according to Flyway's naming convention (if relevant)
  • You've included a header in any source code files (if relevant)

package.json Outdated
Comment on lines 1 to 8
{
"dependencies": {
"@types/react": "^18.0.25",
"i18next": "^22.0.6",
"i18next-browser-languagedetector": "^7.0.1",
"react-i18next": "^12.0.0"
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This should be inside the existing package.json file inside the 'web` directory.

Just add these to the file:

    "i18next": "^22.0.6",
    "i18next-browser-languagedetector": "^7.0.1",
    "react-i18next": "^12.0.0"

and run npm i

Copy link
Member

Choose a reason for hiding this comment

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

This file along with the lock file should then be deleted.

web/src/i18n.js Outdated
}
});

export default i18n;
Copy link
Member

Choose a reason for hiding this comment

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

You can apply formatting by running npm run eslint-fix that will cleanup newline issues like this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ran eslint with npx, think I resolved this

web/src/i18n.js Outdated
@@ -0,0 +1,46 @@
import i18n from 'i18next';
Copy link
Member

Choose a reason for hiding this comment

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

Can you make this a TypeScript file? End it with the *.ts instead of the JavaScript extension.

@codecov
Copy link

codecov bot commented Nov 18, 2022

Codecov Report

Merging #2254 (70d7cf2) into main (1f3e5da) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #2254   +/-   ##
=========================================
  Coverage     77.01%   77.01%           
  Complexity     1166     1166           
=========================================
  Files           222      222           
  Lines          5307     5307           
  Branches        424      424           
=========================================
  Hits           4087     4087           
  Misses          747      747           
  Partials        473      473           

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

@merobi-hub merobi-hub force-pushed the ui/add-i18next branch 6 times, most recently from 0a368ca to 5c6e024 Compare November 19, 2022 03:20
@merobi-hub
Copy link
Collaborator Author

@phixMe reverting to i18n.js temporarily because the .ts version was causing the build to fail. Here's what it looks like when I set my system language to French:
Screen Shot 2022-11-18 at 22 18 04

@merobi-hub merobi-hub force-pushed the ui/add-i18next branch 3 times, most recently from 18d2687 to 5997e68 Compare November 19, 2022 21:22
@merobi-hub
Copy link
Collaborator Author

When the translation is longer than the original, the effect is not great, unfortunately. But there's no reason to assume these "translations" (from Google) are worth keeping. @julienledem @mobuchowski would you please review?
Screen Shot 2022-11-19 at 16 17 30

web/src/i18n.js Outdated
jobs: 'Pracy',
and: 'i',
datasets: 'Zestawy Danych'
}
Copy link
Contributor

@mobuchowski mobuchowski Nov 21, 2022

Choose a reason for hiding this comment

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

I think we need to agree on run, job, dataset translations. IMO

RUN: wykonanie (as in "wykonanie zadania")
JOB: zadanie
DATASET: zbiór danych

I think only wykonanie might be controversial - it translates more to execution, but I don't have better idea. Bieg or przebieg make sense only as the sport IMO.

then:

docs_link: Dokumentacja API
latest_tab: OSTATNIE WYKONANIE
history tab: HISTORIA WYKONAŃ
location: LOKALIZACJA
empty_title: Brak dostępnych informacji o wykonaniu
empty_body: Spróbuj dodać kilka wykonań dla tego zadania

search: Wyszukiwanie
jobs: Zadania
and: i
datasets: Zbiory Danych

Still, translations of "computer concepts" will always sound awkward in Polish for me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say wykonanie sounds more natural to me. We shouldn't be so strict about translating this one to one. Job and dataset are just right.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, @mobuchowski and @JDarDagran . I've revised the translations.

@mobuchowski
Copy link
Contributor

Does it add an option to switch language or does it force a translation based on something like browser language?

I think it would be great to have language switcher if we do something like it.

@merobi-hub
Copy link
Collaborator Author

@mobuchowski i18next does support a language-switcher setup, but that route is a bit more labor-intensive.

@merobi-hub merobi-hub force-pushed the ui/add-i18next branch 2 times, most recently from 52dac32 to 3745a7e Compare November 21, 2022 18:40
@merobi-hub
Copy link
Collaborator Author

@phixMe To translate the text in web/src/components/lineage/Lineage.tsx I did a workaround that required changing the tsconfig.json to set 'noImplicitAny' to 'false'. The initial problem was that there doesn't seem to be any way to use the useTranslation function (const {t} = useTranslation()), as in the other pages. I don't intend to leave this change to tsconfig. Would you please take a look at Lineage.tsx when you have time?

"webpack-merge": "^4.2.1",
"i18next": "^22.0.6",
"i18next-browser-languagedetector": "^7.0.1",
"react-i18next": "^12.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

There should be an update to the lock file in this directory.

@@ -23,6 +23,8 @@ import MqEmpty from '../core/empty/MqEmpty'
import MqText from '../core/text/MqText'
import Node from './components/node/Node'
import ParentSize from '@visx/responsive/lib/components/ParentSize'
import { useTranslation } from 'react-i18next'
import i18next from '../../i18n'
Copy link
Member

Choose a reason for hiding this comment

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

@merobi-hub I think you don't want to be importing this here. This should be your type issue for no-implicit-any

Comment on lines 116 to 137
<FormControl variant='outlined' className={classes.formControl}>
<Select
value={i18next.resolvedLanguage}
onChange={event => {
changeLanguage(event.target.value as string)
}}
input={<MqInputBase />}
>
<MenuItem key={'en'} value={'en'}>
{'en'}
</MenuItem>
<MenuItem key={'es'} value={'es'}>
{'es'}
</MenuItem>
<MenuItem key={'fr'} value={'fr'}>
{'fr'}
</MenuItem>
<MenuItem key={'pl'} value={'pl'}>
{'pl'}
</MenuItem>
</Select>
</FormControl>
Copy link
Member

Choose a reason for hiding this comment

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

How is this looking now? Can you add a screenshot to the PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Screen Shot 2022-12-05 at 18 16 40

@@ -11,7 +11,8 @@
"lib": ["es2016", "es2017", "dom"],
"skipLibCheck": true,
"esModuleInterop": true,
"downlevelIteration": true
"downlevelIteration": true,
"resolveJsonModule": true
},
"include": ["./src/**/*"],
"exclude": ["./src/__tests__/*"]
Copy link
Member

Choose a reason for hiding this comment

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

Can we do without these config changes now? Implicit any in TypeScript is not the best rule to have disabled.

@@ -22,7 +22,7 @@ const webpackDev = {
},
proxy: {
'/api': {
target: `http://${process.env.MARQUEZ_HOST || 'localhost'}:${process.env.MARQUEZ_PORT || 8080}/`,
target: `http://${process.env.MARQUEZ_HOST || 'localhost'}:${process.env.MARQUEZ_PORT || 5000}/`,
Copy link
Member

Choose a reason for hiding this comment

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

I think we don't want this change committed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

where's the facepalm emoji? (reverted this)

@merobi-hub
Copy link
Collaborator Author

Screen Shot 2022-12-05 at 18 13 25

Signed-off-by: Michael Robinson <[email protected]>
@merobi-hub
Copy link
Collaborator Author

Screen Shot 2022-12-06 at 13 42 03
Screen Shot 2022-12-06 at 13 42 39
Screen Shot 2022-12-06 at 13 43 07
Screen Shot 2022-12-06 at 13 43 36
Screen Shot 2022-12-06 at 13 44 09

@merobi-hub
Copy link
Collaborator Author

@phixMe I believe this is RFR again. I added some missing translations I found, caught up to the new Events route and reverted the configuration changes. See the screenshots for what it looks like.

phix and others added 3 commits December 6, 2022 13:31
# Conflicts:
#	web/package-lock.json
#	web/src/components/header/Header.tsx
#	web/src/components/jobs/JobDetailPage.tsx
#	web/src/components/jobs/RunInfo.tsx
#	web/src/components/jobs/Runs.tsx
#	web/src/components/lineage/Lineage.tsx
#	web/src/components/search/SearchPlaceholder.tsx
#	web/src/components/sidenav/Sidenav.tsx
#	web/src/i18n/config.ts
#	web/src/types/i18next.d.ts
#	web/tsconfig.json
@merobi-hub
Copy link
Collaborator Author

merobi-hub commented Dec 7, 2022

@phixMe This is RFR. Added a reload after a language change and fixed the reloading issue so the new language setting persists.

Signed-off-by: Michael Robinson <[email protected]>
@merobi-hub merobi-hub force-pushed the ui/add-i18next branch 2 times, most recently from a1d7c8e to be600e7 Compare December 7, 2022 20:58
@merobi-hub
Copy link
Collaborator Author

OK, now this is actually RFR @phixMe

Copy link
Member

@phixMe phixMe left a comment

Choose a reason for hiding this comment

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

Looks great @merobi-hub. Thanks for the contribution.

src="https://raw.githubusercontent.com/MarquezProject/marquez/main/web/src/img/iconSearchArrow.svg"
width={'30px'}
/>
<SVG src='../../img/iconSearchArrow.svg' width={'30px'} />
Copy link
Member

Choose a reason for hiding this comment

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

@merobi-hub, this reverts the change for #2280

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

Successfully merging this pull request may close these issues.

5 participants