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

Rename mail and change order #44

Open
wants to merge 12 commits into
base: development
Choose a base branch
from

Conversation

Kaushik-Iyer
Copy link
Contributor

This PR closes/references issue #11

How has this been tested?

Results on running the server locally are:

image

Checklist

  • I have added tests to cover my changes.
  • I have updated the documentation.
  • My change is listed in the doc/changelog.rst.

Co-authored-by: Marco A. Gutierrez <marco@openrobotics.org>
Co-authored-by: Marco A. Gutierrez <marco@openrobotics.org>
Co-authored-by: Marco A. Gutierrez <marco@openrobotics.org>
@Kaushik-Iyer
Copy link
Contributor Author

Do I have to change anything in this PR? @marcoag

@marcoag marcoag self-requested a review February 22, 2024 06:40
@marcoag
Copy link
Member

marcoag commented Feb 22, 2024

Yes, I think some suggestion I made on #35 might have not been applied here, so please have a look, i.e.: e-mail or email consistency (choose one and use it through the changes)

  • Review Portuguese from protugal, doesn't seem right.
  • I see a change for {% translate "Message Center" %} but no change of Message Center on any translation, shouldn't this come along with changes on the .po files?

Copy link
Member

@marcoag marcoag left a comment

Choose a reason for hiding this comment

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

Requesting changes, please take a look at my comment above.

@Kaushik-Iyer
Copy link
Contributor Author

I'm pretty sure I had implemented all of your suggestion in the previous PR, but I'll still have a look. But for changing email/mail translation and the message center, how do I decide what's right? should I just google translate and make the change?

@marcoag
Copy link
Member

marcoag commented Feb 22, 2024

You're mixing thins:

  • for e-mail/email is a suggestion I made on the previous PR: there's multiple uses of e-mail and email, both are correct but you should stick to one through all the translations. Maybe check what's on the other translations and use that one.

Since you missed this suggestion I asked you to also double check no others are missing here.

  • for the message center you are changing this:
-                                        {% translate "Mails" %}
+                                        {% translate "Message Center" %}

Which basically changes the translation msgid Mails to Message Center. I am assuming this should come with changes on the .po files so what was being translated as Mails should also now be translated as Message Center. Also be careful and check through the code base for other instances of Mails as they will have to be changed to Message Center.

@marcoag
Copy link
Member

marcoag commented Feb 22, 2024

For the Portuguese from portugal weird translation you can probably use some decent online translation tool.

@Kaushik-Iyer
Copy link
Contributor Author

online tools are suggesting the same translations for Portuguese (Portugal), except some which suggest 'Redigir Emails' instead of 'Compor Emails' for Compose. And as for the discrepancy between email and e-mail, even other translations are very random between the two

@marcoag
Copy link
Member

marcoag commented Feb 22, 2024

Ok for portuguese.

For the e-email vs email check the most common one around and use that one in this PR, at least lets start to be consistent as long as we can.

And resolve conflict, plz.

Also please check for other appearances of the modified msgids around the codebase, if there are appearances they will have to also be changed.

@Kaushik-Iyer
Copy link
Contributor Author

I checked and saw email is more common, so used email in my changes.
checked for other occurrences of msgid "Mails" and changed them to message center as well, and solved conflicts

@Kaushik-Iyer
Copy link
Contributor Author

@marcoag anything else to be changed?

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.

None yet

3 participants