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

Fix #3930 Greeting text on home screen should be a single TextView #5131

Merged
merged 16 commits into from
Oct 6, 2023
Merged

Fix #3930 Greeting text on home screen should be a single TextView #5131

merged 16 commits into from
Oct 6, 2023

Conversation

ShubhadeepKarmakar
Copy link
Collaborator

@ShubhadeepKarmakar ShubhadeepKarmakar commented Aug 13, 2023

Explanation

Removes profile_name_text_view and combines the greeting text and profile name using one string template, welcome_profile_name in WelcomeViewModel.computeWelcomeText()

In the testHomeActivity_initialArabicContext_displaysStringsInArabic test, unicode characters \u200F\u202A...\u202C\u200F! are introduced to allow bidirectional reading.

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

If your PR includes UI-related changes, then:

  • For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)

RTL:

arabic rtl

LTR:

ltr

@ShubhadeepKarmakar
Copy link
Collaborator Author

@adhiamboperes am I going the right way?

Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

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

Thanks @ShubhadeepKarmakar!

I have left some comments inline.

Here is a resource on Localization in android thet might help.

app/src/main/res/layout/welcome.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values-sw/strings.xml Outdated Show resolved Hide resolved
@ShubhadeepKarmakar
Copy link
Collaborator Author

ShubhadeepKarmakar commented Aug 14, 2023

Now just need to customize corresponding tests?

@adhiamboperes
Copy link
Collaborator

Now just need to customize corresponding tests?

Yes please, make sure the tests pass.

Also please address, in your PR description, the concerns raised in the issue description regarding LTR and the underline text.

@ShubhadeepKarmakar
Copy link
Collaborator Author

@adhiamboperes please check it.

Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

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

Thanks @ShubhadeepKarmakar!

I have one comment to help fix your failing test.

Once all your CI checks pass, please assign this PR to Ben by @ mentioning him with "(@)Ben Henning, PTAL".

@ShubhadeepKarmakar
Copy link
Collaborator Author

@adhiamboperes can you help me to get out of these 4 failed checks?

@MohitGupta121
Copy link
Member

MohitGupta121 commented Aug 22, 2023

@ShubhadeepKarmakar actually your one of the Robolectric test is failing as String in Arabic is not matching with exact text. You have to change the text as per the log excepted here in this test.

For more details go through this wiki

@ShubhadeepKarmakar
Copy link
Collaborator Author

@MohitGupta121 thanks but still two checks failed.

@oppiabot
Copy link

oppiabot bot commented Aug 29, 2023

Hi @ShubhadeepKarmakar, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 29, 2023
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 30, 2023
@adhiamboperes
Copy link
Collaborator

Hi @ShubhadeepKarmakar, please let me know if you need any further assistance with this PR.

@oppiabot
Copy link

oppiabot bot commented Sep 19, 2023

Hi @ShubhadeepKarmakar, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Sep 19, 2023
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Sep 20, 2023
@adhiamboperes
Copy link
Collaborator

@ShubhadeepKarmakar please re-assign this PR to me once you have addressed all the comments.

@ShubhadeepKarmakar
Copy link
Collaborator Author

Hi @ShubhadeepKarmakar, please let me know if you need any further assistance with this PR.

Then there will be no space between greeting and profile name.

@adhiamboperes
Copy link
Collaborator

Hi @ShubhadeepKarmakar, please let me know if you need any further assistance with this PR.

Then there will be no space between greeting and profile name.

There will be, if you put a space between the two %s in the template. Have you tried the solution?

@ShubhadeepKarmakar ShubhadeepKarmakar requested a review from a team as a code owner September 28, 2023 14:47
@ShubhadeepKarmakar
Copy link
Collaborator Author

Hi @ShubhadeepKarmakar, please let me know if you need any further assistance with this PR.

Then there will be no space between greeting and profile name.

There will be, if you put a space between the two %s in the template. Have you tried the solution?

Thanks @adhiamboperes. Sorry it was my fault, I changed the welcome() function like you said.

Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

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

Thanks @ShubhadeepKarmakar, LGTM.

@oppiabot
Copy link

oppiabot bot commented Sep 28, 2023

Assigning @BenHenning for code owner reviews. Thanks!

@oppiabot
Copy link

oppiabot bot commented Oct 5, 2023

Hi @ShubhadeepKarmakar, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Oct 5, 2023
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Oct 6, 2023
@adhiamboperes adhiamboperes merged commit 7d0a594 into oppia:develop Oct 6, 2023
36 checks passed
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.

Greeting text on home screen should be a single TextView
4 participants