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.

Sign up for GitHub

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

Jump to bottom

Fix #3561 : Merged 2 version of profile-list-profile-view-xml into single xml #4108

Merged
merged 3 commits into from
Feb 14, 2022
Merged

Fix #3561 : Merged 2 version of profile-list-profile-view-xml into single xml #4108

merged 3 commits into from
Feb 14, 2022

Conversation

Aakash1121
Copy link
Contributor

@Aakash1121 Aakash1121 commented Jan 18, 2022 •
edited

Explanation

Fixes #3561 : Merged portrait and landscape version profile-list-profile-view-xml of Edit-Profiles under Profile-Management.

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
  • 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

Phone Before:

Phone_portrait_before
Phone_landscape_before


Phone After:

phone_portrait_after
phone_landscape_after

Tablet Before:

tablet_landscape_before
tablet_portrait_before


Tablet After:

tablet_portrait_after
tablet_landscape_after

@Aakash1121 Aakash1121 changed the title Merged 2 versions of profile-list-profile-view-xml into one xml Fixes #3561 : Merged 2 version of profile-list-profile-view-xml into single xml Jan 18, 2022
@Aakash1121 Aakash1121 changed the title Fixes #3561 : Merged 2 version of profile-list-profile-view-xml into single xml Fix #3561 : Merged 2 version of profile-list-profile-view-xml into single xml Jan 18, 2022
@rt4914 rt4914 self-assigned this Jan 19, 2022
@Aakash1121
Copy link
Contributor Author

@rt4914 @yash10019coder , Can you let me know if I had solved the issue properly or not.
And am I still assigned to this issue? as @rt4914 self-assigned him.
Am a beginner, help me out with this.

@rt4914
Copy link
Contributor

rt4914 commented Jan 19, 2022

@rt4914 @yash10019coder , Can you let me know if I had solved the issue properly or not. And am I still assigned to this issue? as @rt4914 self-assigned him. Am a beginner, help me out with this.

@Aakash1121 I will review it tonight.

rt4914
rt4914 suggested changes Jan 19, 2022
View reviewed changes
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.

@Aakash1121 PTAL at suggestions. Thanks.

app/src/main/res/layout/profile_list_profile_view.xml Outdated
@@ -36,6 +36,7 @@
android:layout_width="0dp"
android:layout_height="wrap_content"
android:layout_marginStart="20dp"
android:layout_marginBottom="10dp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this value added ? It was not available earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The profile image and profile name were not in symmetric order. So I added 10dp of marginBottom to the profile name textview.

Copy link
Contributor

Choose a reason for hiding this comment

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

But that should be filed as a separate issue and then solved. As this PR is focused on merging of files only and therefore this change can create confusion. Also we generally do not write random values in xml file, we check them from our design files and from that we get the correct value and use that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @rt4914 for guiding, I will make the required changes.

@rt4914 rt4914 assigned Aakash1121 and unassigned rt4914 Jan 19, 2022
@Aakash1121
Copy link
Contributor Author

Hi @rt4914 , can you review the pull request.
Also I have commented on the past review, please check it out.

@rt4914 rt4914 assigned rt4914 and unassigned Aakash1121 Jan 21, 2022
@Aakash1121
Copy link
Contributor Author

Hi @rt4914 , any updates on this issue. Do I have to do something further in this? Please let me know.

@rt4914
Copy link
Contributor

rt4914 commented Jan 25, 2022

@Aakash1121 Just replied here #4108 (comment)

@rt4914 rt4914 assigned Aakash1121 and unassigned rt4914 Jan 25, 2022
@Aakash1121
Copy link
Contributor Author

Hi @rt4914, I have made the required changes, can you review it.

@Aakash1121
Copy link
Contributor Author

Hi @rt4914 , any updates for this issue?

@rt4914 rt4914 assigned rt4914 and unassigned Aakash1121 Feb 2, 2022
@rt4914
Copy link
Contributor

rt4914 commented Feb 2, 2022

@Aakash1121 Please make sure that you reach out to your mentor @bkaur-bkj so that review the PR first and also assign it to me for review when ready.

rt4914
rt4914 suggested changes Feb 2, 2022
View reviewed changes
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.

@Aakash1121 Thanks. Only one change suggested. Rest all is good.

app/src/main/res/values-land/dimens.xml Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned Aakash1121 and unassigned rt4914 Feb 2, 2022
rt4914
rt4914 approved these changes Feb 2, 2022
View reviewed changes
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.

@oppiabot oppiabot bot added the PR: LGTM label Feb 2, 2022
@Aakash1121
Copy link
Contributor Author

Hi @rt4914 @bkaur-bkj , are there any steps left for merging the pull request?. Please let me know.

@bkaur-bkj
Copy link
Contributor

Hi @rt4914 @bkaur-bkj , are there any steps left for merging the pull request?. Please let me know.

HI @Aakash1121 Your PR is approved it will be merged by @rt4914

@bkaur-bkj
Copy link
Contributor

@rt4914 PTAL

@bkaur-bkj bkaur-bkj removed their assignment Feb 3, 2022
@Aakash1121
Copy link
Contributor Author

Hi @rt4914 , can you merge this pull request, if it's proper?

@Aakash1121
Copy link
Contributor Author

Hi @rt4914 @bkaur-bkj any updates for this pull request?

@rt4914 rt4914 assigned rt4914 and bkaur-bkj and unassigned Aakash1121 Feb 14, 2022
@rt4914 rt4914 merged commit ecf6a32 into oppia:develop Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rt4914 rt4914 rt4914 approved these changes

Assignees

@rt4914 rt4914

@bkaur-bkj bkaur-bkj

Labels
PR: LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Merge profile_list_profile_view.xml into single xml file
3 participants
@Aakash1121 @rt4914 @bkaur-bkj