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

First edits for russian locale #5191

Merged
merged 1 commit into from
Sep 23, 2024
Merged

Conversation

dunmaksim
Copy link
Contributor

Hello!

Now I would like to learn more about the review and localization process on the project. Therefore, localization is only partial.

Thanks!

@app-sre-bot
Copy link

Can one of the admins verify this patch?

@himdel
Copy link
Collaborator

himdel commented Aug 28, 2024

hey :)
you don't need to commit the changes to locale/ru.js, there's weekly automation to update these based on the .po files, but since it's a new file, no harm updating it either :)

If you're updating the list of languages in lingui.config.js, remember to also update src/l10n.ts - the availableLanguages array and languageNames - that's what the UI uses to actually display anything.

Other than that, looking good, you can also sanity-check the .po file by running lint-po (from pypi) - npm run lint:po does that.

(And also note that anything coming from the API would still be in English, unless you also add the new language to ansible/galaxy_ng for the backend.)

(repo tests are currently red, fair warning, actually merging things might take a while)

@dunmaksim
Copy link
Contributor Author

Hello, @himdel !

Me update PR:

  • fixed one error in ru.po after linting with npm run lint:po;
  • translated more strings;
  • commits squashed.

@dunmaksim dunmaksim force-pushed the russian-locale branch 3 times, most recently from 9e533cb to 1a1ff57 Compare September 11, 2024 06:30
@dunmaksim
Copy link
Contributor Author

Hello, @himdel !

What I must do for merging this PR into master?

@himdel
Copy link
Collaborator

himdel commented Sep 18, 2024

Oh, I'm sorry I didn't realize it was ready..
the code looks good 👍 thanks!,
kicking the linter workflow...

@himdel
Copy link
Collaborator

himdel commented Sep 18, 2024

Oh, two things please...

  • we have a no merge commits policy, can you squash the changes please? (or remove the merge commit)
  • can you rebase on top of current master please? (careful about rebasing with merge commits though) There were some pipeline fixes that should make the tests go green :)

@dunmaksim
Copy link
Contributor Author

  1. All my commits are squashed into one.

  2. master branch on my fork is updated. This branch is rebased over him.

  3. Local run npm run lint:po is success:

    > [email protected] lint:po
    > lint-po locale/*.po
    

How to run additional checks locally for quality improvements?

@himdel
Copy link
Collaborator

himdel commented Sep 18, 2024

Thanks, I have to trigger the workflow manually for new contributors,
looks like npm run prettier should fix the pr-checks issue,
maybe also run npm run gettext:extract && npm run gettext:compile, it should auto-run over the weekend but looks like it doesn't match now.
And Cypress is likely to pass, I'll try to restart any random failures :)

@dunmaksim
Copy link
Contributor Author

Hello, @himdel !

Thanks for your answers!

Me run manually npm run prettier and other commands from your commentary. PR is updated.

@himdel
Copy link
Collaborator

himdel commented Sep 19, 2024

Thanks for doing all that!
This is now ready to be merged, don't worry about the screenshots test, that's expected, I'll take it from here :)
(hoping to merge later today)

@himdel himdel merged commit 64ef614 into ansible:master Sep 23, 2024
10 of 11 checks passed
@dunmaksim dunmaksim deleted the russian-locale branch September 25, 2024 06:29
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.

3 participants