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 #4015 : User name TextView not properly aligned in profile_edit_fragment.xml Screen fix #4023

Merged
merged 5 commits into from
Jan 18, 2022

Conversation

ihrishix
Copy link
Contributor

@ihrishix ihrishix commented Dec 2, 2021

Explanation

Fixes #4015
Text View wasn't constrained to the right and was set to wrap_content, which made text to come out of the view.
Setting width to 0dp, And constraining it's right to right of the parent view, with some padding solves the issue.
Adding Text Direction : Locale, fixes the gravity issue

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:

  • Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes

Before Portrait
before_portrait

After Portrait
after_portrait

Before Landscape
before_landscape

After Landscape
after_landscape

RTL Before Portrait

before_rtl_portrait

RTL After Portrait

after_rtl_portrait (2)

RTL Before Landscape

before_rtl_landscape

RTL After Landscape

after_rtl_landscape

  • For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
  • Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
  • Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

@rt4914
Copy link
Contributor

rt4914 commented Dec 8, 2021

@rushikeshsuryawanshi Considering you are changing the UI. Please add RTL screenshots too. https://github.com/oppia/oppia-android/wiki/RTL-Guidelines#testing-app-for-rtl-layouts

@bkaur-bkj
Copy link
Contributor

@rushikeshsuryawanshi add a line of explaination in the PR explaining your idea and approach , also mark the checklist as [x] remove the space so that tick mark is shown.Take care to pick the issues after anyone has assigned you as there may be a possibility of someone else working on it. Fore present I have assigned this issue to you.

@bkaur-bkj
Copy link
Contributor

@rushikeshsuryawanshi also you can take reference of other PR while writing title or opening comment, like here you need to write Fix #issue_number: and then PR title ( ref - #4004 )

@ihrishix ihrishix changed the title Fixed issue #4015 Fix #4015 : User name TextView not properly aligned in profile_edit_fragment.xml Screen fix Dec 8, 2021
@ihrishix
Copy link
Contributor Author

ihrishix commented Dec 8, 2021

@bkaur-bkj @rt4914

I've made changes as per your instruction. It's my First Contribution.
Could you please merge the PR ?

@rt4914
Copy link
Contributor

rt4914 commented Dec 8, 2021

@rushikeshsuryawanshi RTL after portrait image does not look correct. The gravity/alignment of the text should be right instead of left.

@ihrishix
Copy link
Contributor Author

ihrishix commented Dec 9, 2021

@rt4914
Adding "TextDirection : locale" fixes that. Made changes as per your instructions. Updated screenshots.

@oppiabot
Copy link

oppiabot bot commented Dec 16, 2021

Hi @rushikeshsuryawanshi, 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 Dec 16, 2021
@ihrishix
Copy link
Contributor Author

@rt4914 Could you please review my PR and merge ?

@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Dec 16, 2021
@bkaur-bkj bkaur-bkj assigned rt4914 and unassigned bkaur-bkj Dec 16, 2021
@bkaur-bkj
Copy link
Contributor

bkaur-bkj commented Dec 16, 2021

@rt4914 Could you please review my PR and merge ?

@rushikeshsuryawanshi if you want your PR to be reviewed you need to assign it back to reviewer. write PTAL @name only then he will know that your PR is to be reviewed

@ihrishix
Copy link
Contributor Author

PTAL @rt4914

@ihrishix
Copy link
Contributor Author

@bkaur-bkj Okay thanks. This is my first PR so, new to these things. Is that correct now ?

@BenHenning
Copy link
Sponsor Member

Hi. As of today, some main reviewers have taken time off for the next few weeks, so it may take a little while before we can look at this PR. We appreciate your patience while some of our team members recharge. We'll be fully returning on 4 January 2021.

@oppiabot
Copy link

oppiabot bot commented Dec 24, 2021

Hi @rushikeshsuryawanshi, 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 Dec 24, 2021
@ihrishix
Copy link
Contributor Author

.

@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Dec 24, 2021
@oppiabot
Copy link

oppiabot bot commented Dec 31, 2021

Hi @rushikeshsuryawanshi, 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 Dec 31, 2021
@ihrishix
Copy link
Contributor Author

.

@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Dec 31, 2021
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

@rushikeshsuryawanshi Apologies for the delayed review most of our teammates were on year-end break. Thanks a lot for being patient.

app/src/main/res/layout/profile_edit_fragment.xml Outdated Show resolved Hide resolved
@rt4914 rt4914 removed their assignment Jan 3, 2022
@ihrishix
Copy link
Contributor Author

PTAL @rt4914

@oppiabot oppiabot bot assigned rt4914 and unassigned ihrishix Jan 11, 2022
@oppiabot
Copy link

oppiabot bot commented Jan 11, 2022

Unassigning @rushikeshsuryawanshi since a re-review was requested. @rushikeshsuryawanshi, please make sure you have addressed all review comments. Thanks!

@rt4914
Copy link
Contributor

rt4914 commented Jan 12, 2022

@rushikeshsuryawanshi Check this comment : #4023 (comment)

@rt4914 rt4914 assigned ihrishix and unassigned rt4914 Jan 12, 2022
@bkaur-bkj
Copy link
Contributor

@rushikeshsuryawanshi make sure to assign the reviewer back after your changes are done. write PTAL @Reviewer name to assign them only then we will be notified

@bkaur-bkj
Copy link
Contributor

and then unassign yourself too, I have done this for now

@bkaur-bkj bkaur-bkj assigned rt4914 and unassigned ihrishix Jan 16, 2022
@ihrishix
Copy link
Contributor Author

@bkaur-bkj Thanks ! I didn't knew this.

Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@rt4914 rt4914 enabled auto-merge (squash) January 17, 2022 19:08
@oppiabot oppiabot bot unassigned rt4914 Jan 17, 2022
@oppiabot
Copy link

oppiabot bot commented Jan 17, 2022

Unassigning @rt4914 since they have already approved the PR.

@oppiabot oppiabot bot added the PR: LGTM label Jan 17, 2022
@bkaur-bkj
Copy link
Contributor

@rt4914 here one Robolectic test is failing, pls do let us know what can be done

@rt4914
Copy link
Contributor

rt4914 commented Jan 18, 2022

Looks like build is failing unexpectedly as it does not point to any specific test failure. Re-running the jobs.

@rt4914 rt4914 merged commit 3d876dc into oppia:develop Jan 18, 2022
bhaktideshmukh pushed a commit to bhaktideshmukh/oppia-android that referenced this pull request Jan 25, 2022
…dit_fragment.xml Screen fix (oppia#4023)

* Fixed issue oppia#4015

* Text Direction Fix

* Removed TextDirection

* Revert "Removed TextDirection"

This reverts commit 462d8b0

* Removed Text Direction. Changed style
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User name TextView not properly aligned in profile_edit_fragment.xml Screen
4 participants