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

feat(ui): Add Hindi localization language support (#7778). #7778

Merged
merged 9 commits into from
Feb 24, 2022
Merged

feat(ui): Add Hindi localization language support (#7778). #7778

merged 9 commits into from
Feb 24, 2022

Conversation

adityathakurxd
Copy link
Contributor

Description

Added localization support for Hindi. Hindi is the main language of the Indian states of Uttar Pradesh, Haryana, Himachal Pradesh, and the capital Delhi in North India; Bihar and Jharkhand, and more.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]).
This will ensure a smooth and quick review process. Updating the pubspec.yaml and changelogs is not required.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (melos run analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

Copy link

@rahulraj64 rahulraj64 left a comment

Choose a reason for hiding this comment

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

Saw some texts yet pending translations.
(I'm not from Google. Just sharing some thoughts)

@@ -0,0 +1,263 @@
import '../default_localizations.dart';

class EnLocalizations extends FlutterFireUILocalizationLabels {

Choose a reason for hiding this comment

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

Hello Aditya, shouldn't it be HiLocalizations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes Sorry about that I'll fix this.

final String referenceLabel;

const EnLocalizations({
this.emailInputLabel = 'Email',
Copy link

@rahulraj64 rahulraj64 Jan 10, 2022

Choose a reason for hiding this comment

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

Seems there are a lot of texts yet to be translated to Hindi :) If this is a work in progress, please consider marking this as a draft :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a question regarding that. I could see some other translations use Email as Email. I thought it'd do for Hindi as well. Should I like translate these too?

Choose a reason for hiding this comment

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

I suggest adding native translations where ever possible. Also, I noticed that you have used English scripts all over the place for translation. Some languages like Bahasa, Spanish, etc use English script by default and this is not the case for Hindi. For example,
Phone number zaroori hai - Wrong
फ़ोन नंबर आवश्यक है - Correct

Please note that I'm not a native Hindi speaker, these are some generic comments and I may not be able to review the translation itself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, would look into that. And, update this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @rahulraj64, I have made the changes. Could you please review it again? Thanks!

this.signOutButtonText = 'Sign out',
this.phoneInputLabel = 'Phone number',
this.phoneNumberInvalidErrorText = 'Phone number is invalid',
this.phoneNumberIsRequiredErrorText = 'Phone number zaroori hai',

Choose a reason for hiding this comment

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

Use proper language script all over your translations. Currently, they are Hindi-In-English (Not proper Hindi translations)

@@ -1,3 +1,4 @@
import 'package:flutterfire_ui/src/i10n/lang/hi.dart';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import 'package:flutterfire_ui/src/i10n/lang/hi.dart';
import 'lang/hi.dart';

@@ -0,0 +1,263 @@
import '../default_localizations.dart';

class EnLocalizations extends FlutterFireUILocalizationLabels {
Copy link
Member

@lesnitsky lesnitsky Jan 13, 2022

Choose a reason for hiding this comment

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

Suggested change
class EnLocalizations extends FlutterFireUILocalizationLabels {
class HiLocalizations extends FlutterFireUILocalizationLabels {

Copy link
Member

@lesnitsky lesnitsky left a comment

Choose a reason for hiding this comment

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

Still several unresolved review comments.
@adityathakurxd would you be able to make relevant changes?

@adityathakurxd
Copy link
Contributor Author

Yes, @lesnitsky I'll update with the required changes. Sorry for the delay.

this.signInWithEmailLinkSentText =
"हमने आपको एक जादुई लिंक के साथ एक ईमेल भेजा है। अपना ईमेल जांचें और साइन इन करने के लिए लिंक का अनुसरण करें",
this.sendLinkButtonLabel = 'मैजिक लिंक भेजें',
this.arrayLabel = 'array',
Copy link
Member

Choose a reason for hiding this comment

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

@adityathakurxd could these (lines 243 to 261) labels be translated as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the same.

@adityathakurxd
Copy link
Contributor Author

Hi, @lesnitsky Could you tell me about the checks? How can I improve for them?

@russellwheatley
Copy link
Member

Hey @adityathakurxd, thanks for the PR! Do you mind resolving the merge conflict, please?

Hey @rahulraj64, do you mind proofreading the update language translations in Hindi and approving if you find acceptable, please? Unfortunately, none of the maintainers I know speak Hindi. Thank you!

@adityathakurxd
Copy link
Contributor Author

Hey @russellwheatley, I have resolved the merge conflicts.

@russellwheatley
Copy link
Member

Hey @adityathakurxd, I don't think the merge conflict has been resolved.

@russellwheatley russellwheatley added the blocked: customer-response Waiting for customer response, e.g. more information was requested. label Feb 22, 2022
@adityathakurxd
Copy link
Contributor Author

Hey @russellwheatley, Sorry about that. I had last resolved the conflicts 8 days ago and #7709 was merged just after that which caused it again. Thank you! Let me know if there's anything else I need to do.

@google-oss-bot google-oss-bot added Needs Attention This issue needs maintainer attention. and removed blocked: customer-response Waiting for customer response, e.g. more information was requested. labels Feb 23, 2022
@russellwheatley
Copy link
Member

Hey @adityathakurxd, do you mind getting a fellow developer who speaks Hindi to proofread & approve to ensure there are no mistakes (We all make them 😄 ), please?

@adityathakurxd
Copy link
Contributor Author

Sure @russellwheatley I'll ask a few developers to review this.

@infiniteoverflow
Copy link

All the translations are perfect.

@adityathakurxd just curious about the following :

verifyPhoneNumberButtonText and linkEmailButtonText are both translated to अगला. Does this have to be like this in this context or can it be changed to फ़ोन नंबर सत्यापित करें and लिंक ईमेल ?

@adityathakurxd
Copy link
Contributor Author

Hey, Aswin (@infiniteoverflow)
Thank you so much for the review! I had translated the original en.dart available here. Both the verifyPhoneNumberButtonText and linkEmailButtonText are button labels that say 'Next' in English so I thought it'd best to translate them to अगला just in case there are further steps to be performed.

Copy link
Member

@russellwheatley russellwheatley left a comment

Choose a reason for hiding this comment

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

LGTM

@russellwheatley russellwheatley changed the title feat(ui): add Hindi localization support feat(ui): Add Hindi localization language support (#7778). Feb 24, 2022
@russellwheatley russellwheatley merged commit b584ce0 into firebase:master Feb 24, 2022
@firebase firebase locked and limited conversation to collaborators Mar 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Needs Attention This issue needs maintainer attention.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants