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

Finish i18n initial integration #691

Merged
merged 25 commits into from
Dec 16, 2024
Merged

Finish i18n initial integration #691

merged 25 commits into from
Dec 16, 2024

Conversation

roll
Copy link
Collaborator

@roll roll commented Dec 13, 2024


@romicolman
A next step would be creating a follow-up issue that covers:

  • language persistence
  • language detection from the system settings
  • testing not in English and catching all the missing translations (might be a separate one)

@roll roll requested review from guergana and romicolman December 13, 2024 10:09
@guergana
Copy link
Collaborator

@roll when i was trying to implement this, I had the same e2e test error. if you are testing locally with the web app or even with make preview it doesn't mean the app build is working. Please test that the proper build is working.

I just checked and the local build in Linux looks great and the translations work perfectly... I noticed the error report translations are missing, but this we knew already, but this line here is also missing: https://github.com/okfn/opendataeditor/blob/main/client/components/Views/Report/Table.tsx#L30 where it says 'Row number', could you add this translation?

Copy link

cloudflare-workers-and-pages bot commented Dec 16, 2024

Deploying opendataeditor with  Cloudflare Pages  Cloudflare Pages

Latest commit: eb70669
Status: ✅  Deploy successful!
Preview URL: https://39534b8d.opendataeditor.pages.dev
Branch Preview URL: https://651-finish-i18n.opendataeditor.pages.dev

View logs

@roll roll requested a review from guergana December 16, 2024 10:57
Copy link
Collaborator

@romicolman romicolman left a comment

Choose a reason for hiding this comment

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

Hi Evgeny!! It looks great! Just one small comment. Is it possible to remove the text "language" at the top of the bottom (if this is not pre-defined, of course).
Captura de pantalla 2024-12-16 a la(s) 1 31 10 p  m

I think it would be better so that all text from features there are the same. Again, if this is not possible, let's merge it.

I checked dialogs, buttons and everything looks OK.

@roll
Copy link
Collaborator Author

roll commented Dec 16, 2024

@romicolman
Thanks!

Let's discuss the label on the planning. It might be better to keep it because I think it's a requirement regarding accessibility that all the input have to have corresponding labels.

@roll roll requested review from guergana and removed request for guergana December 16, 2024 14:26
@romicolman
Copy link
Collaborator

Ok, perfect! Let's merge it for now :)

@roll roll merged commit 37b024b into main Dec 16, 2024
9 checks passed
@roll roll deleted the 651/finish-i18n branch December 16, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement language button and finish i18n
3 participants