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

Settings subsections refined w/ translation addition #573

Merged
merged 13 commits into from
Dec 3, 2022

Conversation

benalleng
Copy link
Contributor

Using #524 as a reference I tuned the settings page to be better organized. Additionally I did a small amount of translation for the settings page as well added some unfinished translations in the french translation.json to make it easier to do translations moving forward where it doesn't yet exist. Finally I added little country flags to the languages.js file. While there are only two languages now I think moving forward having a quick way for people to see the flag reference as the list of languages gets longer it could be helpful to use identifying flags.

image

image

@benalleng
Copy link
Contributor Author

Feedback is welcome! I'll start.. Even though the user is led there by a classic cog symbol, I wonder if it would be worth keeping a main "Settings" title at the very top before the first section

@benalleng benalleng changed the title Development Settings subsections refined Dec 1, 2022
@benalleng benalleng changed the title Settings subsections refined Settings subsections refined w/ translation addition Dec 1, 2022
@theborakompanioni
Copy link
Collaborator

Hey @benalleng!

Really nice. Thank you!

Two things from my side:

  • If you are okay with it, let's remove the flags from the language dropdown. e.g. French is spoken in many countries, not exclusively in France. Same with English and many other languages. I'd vote for removing them. Just let me know what you think.
  • Some translations in fr/translation.json are in English. It's easier to see the progress on Transifex, if this file only contains actually translated keys, e.g. French has 68% translated keys. Missing keys will fallback to English anyway!

Feedback is welcome! I'll start.. Even though the user is led there by a classic cog symbol, I wonder if it would be worth keeping a main "Settings" title at the very top before the first section

My 2 sats: I think both is fine. We can keep it this way for now and if you are not happy with it, add a main header in a follow-up PR, if you so please!

What do you think? Let me know if you want to make the adaptions yourself.
Once done, I think it is ready to be merged. 🚀

@benalleng
Copy link
Contributor Author

  • Ok! sounds good to me, as I look at it more I agree that as long as we continue to use the proper script for each language, flags aren't really needed or preferred.
  • sorry about the Transifex thing, I didn't realize that was the proper method to go through translations. For the future I'll make sure to help with translations there 😅 I'll also remove the other translations that already fallback to english!

@theborakompanioni
Copy link
Collaborator

theborakompanioni commented Dec 3, 2022

  • Ok! sounds good to me, as I look at it more I agree that as long as we continue to use the proper script for each language, flags aren't really needed or preferred.

👍

  • sorry about the Transifex thing, I didn't realize that was the proper method to go through translations. For the future I'll make sure to help with translations there sweat_smile I'll also remove the other translations that already fallback to english!

Don't worry!

Two small suggestions (one typo keeping the tests from succeeding, and removing left-over untranslated keys from fr), then we can push it over the finish line. 💪

Copy link
Collaborator

@theborakompanioni theborakompanioni left a comment

Choose a reason for hiding this comment

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

I just now saw, that I can apply the suggestions myself via the UI.
Thank you @benalleng for your work 🙏

Looking forward to your next contribution 🧡

Closes #524.

@theborakompanioni theborakompanioni merged commit 495dc2b into joinmarket-webui:master Dec 3, 2022
@benalleng
Copy link
Contributor Author

Awesome! exciting to have my code merged! A question for the future, how do I conduct tests before I submit a pull request so that i can be certain you have less work to complete the merges on your end. Still a pretty new coder and I'm not too familiar with running tests 😅 Thanks for your help in getting it all finished, excited to sink my teeth into more issues moving forward!

@theborakompanioni
Copy link
Collaborator

Awesome! exciting to have my code merged!

💪

Thanks again for your work. Every little improvement is important in open source development. Your contribution is invaluable for projects like Jam.

A question for the future, how do I conduct tests before I submit a pull request so that i can be certain you have less work to complete the merges on your end. Still a pretty new coder and I'm not too familiar with running tests sweat_smile Thanks for your help in getting it all finished, excited to sink my teeth into more issues moving forward!

Don't worry too much. We can figure it out during review and no code is perfect at first try. You can try running npm test before submitting your work - it's cool that you already adapted the tests in this PR in the first place ;-)

To run all tests (non-interactively) do CI=true npm test.

@dergigi
Copy link
Contributor

dergigi commented Dec 9, 2022

Fantastic! Thank you for your contribution @benalleng 🙏🧡

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants