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.

Sign up for GitHub

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

Jump to bottom

Improved translations for NL and NL_BE #1830

Closed
wants to merge 7 commits into from
Closed

Improved translations for NL and NL_BE #1830

wants to merge 7 commits into from

Conversation

DjedenOfficial
Copy link
Contributor

I improved the translations for NL and NL_BE. Now it has been fully translated instead of partly. The changes haven't been tested. I would like to know how to test since I don't know how (I am new to GitHub).

@ia
Copy link
Collaborator

ia commented Oct 22, 2023

I would like to know how to test since I don't know how

Hello. Once this workflow will be approved to run tests & to prepare builds1, and if everything is ok, then you will see pre-built binaries as downloadable artifacts here which you can download, flash & test on hardware which you have.

Footnotes

  1. Usually it happens automatically once any pull-request is made but it seems something has been changed recently on that so now it seems like admins of the project have to trigger it manually. ↩

@Ralim
Copy link
Owner

Ralim commented Oct 22, 2023

Each new contributor needs to be approved once before the automated actions can run.

So now they should always work for you @DjedenOfficial.

Once they finish you should be able to download the compiled code from them for testing

@ia
Copy link
Collaborator

ia commented Oct 22, 2023 •
edited
Loading

I'm not an expert on git/github myself, but maybe another reason it's because you did make a commit not into a separate branch with different name (after you forked the repo) but into dev branch (which is "protected" branch because it's "upstream" one).

Bottom line is just a suggestion for the future, here is the recommended way to make pull-requests, @DjedenOfficial:

  • fork the repo to your account (i.e., using github interface)1;
  • clone forked repo locally: git clone ...1;
  • create your own branch for your own changes (it seems this step has been skipped - that's why actions maybe haven't been triggered automatically): git checkout -b mychange dev, where mychange - name of your branch, dev - name of upstream branch of forked repo;
  • add changes / fixes by editing files;
  • make a commit: git commit ...;
  • push your local branch with your local changes to your forked repo on github: git push -u YOUR_GITHUB_USERNAME mychange;
  • after that once you open github it will show you that you did make a push into forked repo and github will suggest you to create pull-request automatically.

Something like that.

Footnotes

  1. these steps should be done only once per each repo/project ↩ ↩2

@ia
Copy link
Collaborator

ia commented Oct 22, 2023

I would like to know how to test

Here you go, now here you can download binaries with your changes to test.

@DjedenOfficial
Fixed some text after testing
f96e21b 
It works after testing. However I fixed some minor issues. A second test is required.
@DjedenOfficial
Copy link
Contributor Author

I would like to know how to test

Here you go, now here you can download binaries with your changes to test.

Thanks testing went well I fixed some minor issues and want to run another test.

@Ralim
Copy link
Owner

Ralim commented Oct 22, 2023 •
edited
Loading

All good, Ill approve until this merges; so don't sweat and just give me a bit to see the email 😁
For reference you can get to the test builds by clicking the green tick marks under the "Show all checks" link at the bottom.

@Ralim
Copy link
Owner

Ralim commented Oct 24, 2023

@DjedenOfficial Are you happy with how this looks now ?

@DjedenOfficial
Copy link
Contributor Author

DjedenOfficial commented Oct 27, 2023 •
edited
Loading

@DjedenOfficial Are you happy with how this looks now ?

Sorry for not reacting. It needs just slight changes but I will be making them as soon as possible and are likely to be done by the afternoon.

@DjedenOfficial
Improved translation for NL and NL V3_BE
d5934a0 
Fully translated NL and NL_BE to dutch.
@Ralim
Copy link
Owner

Ralim commented Oct 27, 2023

No rush, just that it went silent so I wasn't sure.

You can compile locally to test; or you can setup your fork to run the builds for testing.

Convention is to mark a PR as a draft until you're good to go (but don't stress, everyone misses the option)

ColoMAX
ColoMAX suggested changes Nov 18, 2023
View reviewed changes
Copy link
Contributor

@ColoMAX ColoMAX left a comment

Choose a reason for hiding this comment

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

From a quick glance, I've found at least these issues.

Translations/translation_NL.json
@@ -4,55 +4,55 @@
"tempUnitFahrenheit": false,
"messagesWarn": {
"CalibrationDone": {
"message": "Calibration\ndone!"
"message": "Calibratie\ngedaan!"
Copy link
Contributor

Choose a reason for hiding this comment

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

Calibratie\nklaar!

Translations/translation_NL.json
},
"ResetOKMessage": {
"message": "Reset OK"
},
"SettingsResetMessage": {
"message": "Instellingen\nzijn gereset!"
"message": "Sommige settings\nzijn veranderd!"
Copy link
Contributor

Choose a reason for hiding this comment

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

Instellingen\nzijn veranderd

Translations/translation_NL.json
},
"NoAccelerometerMessage": {
"message": "Geen accelerometer\ngedetecteerd!"
"message": "Geen accelerometer\ngedectecteerd!"
Copy link
Contributor

Choose a reason for hiding this comment

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

old was correct

Translations/translation_NL.json
},
"LockingKeysString": {
"message": "GEBLOKKEERD"
"message": "LOCKED"
Copy link
Contributor

Choose a reason for hiding this comment

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

Old was correct

Translations/translation_NL.json
},
"UnlockingKeysString": {
"message": "GEDEBLOKKEERD"
"message": "UNLOCKED"
Copy link
Contributor

Choose a reason for hiding this comment

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

Old was correct

Translations/translation_NL.json
},
"UndervoltageString": {
"message": "Onderspanning\n"
},
"InputVoltageString": {
"message": "Voeding V: \n"
"message": "Voedingsspanning: \n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Too long: "Ingangs V: \n"

Translations/translation_NL.json
@@ -70,43 +70,43 @@
"message": "Cooldown\n"
},
"DeviceFailedValidationWarning": {
"message": "Jouw toestel is wellicht een namaak-versie!"
"message": "Jou apparaat is waarschijnlijk namaak!"
Copy link
Contributor

Choose a reason for hiding this comment

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

+"...een namaak!"

Translations/translation_NL.json
"description": ""
},
"UIMenu": {
"displayText": "Weergave\ninstellingen",
"displayText": "Gebruikers-\ninterface",
Copy link
Contributor

Choose a reason for hiding this comment

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

Old was more Dutch sounding

Translations/translation_NL.json
@@ -117,199 +117,199 @@
"menuOptions": {
"DCInCutoff": {
"displayText": "Spannings-\nbron",
"description": "Spanningsbron. Stelt drempelspanning in. (DC 10V) (S 3.3V per cel)"
"description": "Minimale toegelate voltage"
Copy link
Contributor

Choose a reason for hiding this comment

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

Undo this

Translations/translation_NL.json
"displayText": "QC\nvoltage",
"description": "Maximaal QC voltage dat gevraagd mag worden"
"displayText": "Vermogen\nwatt",
"description": "Vermogen van de adapter"
Copy link
Contributor

Choose a reason for hiding this comment

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

QC is a name, you can not translate this away

@discip
Copy link
Collaborator

discip commented Nov 20, 2023

@DjedenOfficial & @ColoMAX
It would be very nice if you could find a solution that you both could agree on, as neither the developer nor anyone else authorized to merge this in are knowledgeable enough to judge what is best. 😊

thanks

@discip discip added the Awaiting Response Waiting for user response, if none issue will be closed. label Nov 29, 2023
@ColoMAX
Copy link
Contributor

ColoMAX commented Dec 4, 2023

Later this week I can try to add a commit to the pr to resolve all

@ColoMAX
Copy link
Contributor

ColoMAX commented Dec 28, 2023

@DjedenOfficial can you add me as a contributor on your fork? I can not push my changes to your repo.
Or is there another way to do it?
I've worked on the Dutch translations only, not the Flemish though, since I'm not familiar with it. Although one might consider, for the time being, to keep it the same as the Dutch until somebody comes around who does, due to the similarities between the two languages/dialects.

@ColoMAX ColoMAX mentioned this pull request Dec 28, 2023
Merged
@discip discip removed the Awaiting Response Waiting for user response, if none issue will be closed. label Jan 4, 2024
@discip discip closed this in #1863 Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ColoMAX ColoMAX ColoMAX requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DjedenOfficial @ia @Ralim @discip @ColoMAX