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

Fixed #1956: Optimise code in profile chooser add view #4844

Merged
merged 13 commits into from
Jan 30, 2023
Merged

Fixed #1956: Optimise code in profile chooser add view #4844

merged 13 commits into from
Jan 30, 2023

Conversation

Uticodes
Copy link
Contributor

@Uticodes Uticodes commented Jan 15, 2023 •
edited by adhiamboperes
Loading

Explanation

Fixed #1956: Optimise code in profile chooser add view

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
Screenshots Before After
Mobile Portrait
mobile_portrait_before mobile_portrait_after
Mobile Landscape
mobile_land_before mobile_land_after
Tab Portrait
tap_portrait_before tap_portrait_after
Tab Landscape
tap_land2_before tap_land2_after
Tab LTR Portrait
tap_lrt_portrait_before tap_ltr_portrait_after
Tap LTR Landscape
tap_ltr_land_before tap_ltr_land_after
Mobile LTR Portrait
mobile_ltr_before mobile_ltr_portrait_after
Mobile LTR Landscape
mobile_ltr_land_before mobile_ltr_land_after

Accessibility Guide

TalkBack.Feature.mov

Unit Test Screenshot

Screenshot 2022-12-09 at 7 52 10 AM

@Uticodes Uticodes requested a review from rt4914 as a code owner January 15, 2023 13:17
@Uticodes
Copy link
Contributor Author

@MohitGupta121 @BenHenning this is the new PR for the the one that was closed because of force push, kindly assign to me and add reviewers thanks.

@MohitGupta121 MohitGupta121 self-requested a review January 15, 2023 14:08
@MohitGupta121 MohitGupta121 self-assigned this Jan 15, 2023
MohitGupta121
MohitGupta121 requested changes Jan 15, 2023
View reviewed changes
Copy link
Member

@MohitGupta121 MohitGupta121 left a comment

Choose a reason for hiding this comment

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

@Uticodes PTAL, if any query feel free to ask.
Thanks.

app/src/main/res/values/styles.xml Outdated Show resolved Hide resolved
app/src/main/res/values/styles.xml Outdated Show resolved Hide resolved
app/src/main/res/values/styles.xml Outdated Show resolved Hide resolved
@Uticodes
Copy link
Contributor Author

Uticodes commented Jan 15, 2023 via email

@Uticodes
Copy link
Contributor Author

@MohitGupta121 I've fixed all comments, kindly review and resolve them if the fix is okay or let me know if there's still any issue to fix. Thanks. 🙏

@MohitGupta121 MohitGupta121 self-requested a review January 17, 2023 01:42
@MohitGupta121
Copy link
Member

MohitGupta121 commented Jan 18, 2023 •
edited
Loading

@Uticodes also comment done on the requested changes comments, so that I'll resolve that.

And please update your PR branch with upstream/develop.

@Uticodes
Copy link
Contributor Author

@Uticodes also comment done on the requested changes comments, so that I'll resolve that.

And please update your PR branch with upstream/develop.

Yes @MohitGupta121 I have attended to all the comments you added. I've also update branch with develop.

MohitGupta121
MohitGupta121 requested changes Jan 18, 2023
View reviewed changes
Copy link
Member

@MohitGupta121 MohitGupta121 left a comment

Choose a reason for hiding this comment

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

@Uticodes PTAL, Thanks.

app/src/main/res/values/styles.xml Outdated Show resolved Hide resolved
app/src/main/res/layout-sw600dp-port/profile_chooser_add_view.xml Show resolved Hide resolved
@MohitGupta121
Copy link
Member

@Uticodes please do this also #4844 (comment)

@Uticodes
Copy link
Contributor Author

@Uticodes please do this also #4844 (comment)

I've fixed that too @MohitGupta121

@MohitGupta121 MohitGupta121 self-requested a review January 18, 2023 11:26
@MohitGupta121 MohitGupta121 assigned rt4914 and unassigned Uticodes Jan 27, 2023
@Uticodes
Copy link
Contributor Author

Suggested changes

Checking

@MohitGupta121 MohitGupta121 assigned Uticodes and unassigned rt4914 Jan 27, 2023
@MohitGupta121
Copy link
Member

@Uticodes please comment done in reviewer comment so it indicates that you complete that change.

@Uticodes
Copy link
Contributor Author

@Uticodes please comment done in reviewer comment so it indicates that you complete that change.

@MohitGupta121 yes will do once done. thanks.

@Uticodes
Copy link
Contributor Author

@rt4914 @MohitGupta121 PTAL, I've made all changes accordingly, kindly review. Thanks.

@Uticodes Uticodes removed their assignment Jan 27, 2023
@Uticodes
Copy link
Contributor Author

@MohitGupta121 please assign the PR to me thanks. mistakingly removed it

@MohitGupta121
Copy link
Member

@Uticodes you left with some changes or all completed?

@MohitGupta121 please assign the PR to me thanks. mistakingly removed it

@Uticodes
Copy link
Contributor Author

@Uticodes you left with some changes or all completed?

@MohitGupta121 please assign the PR to me thanks. mistakingly removed it

all completed

rt4914
rt4914 approved these changes Jan 30, 2023
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, @Uticodes Please reply to all the earlier comments that I have left so that I can fully understand how to fixed those issues.

@rt4914 rt4914 assigned Uticodes and unassigned rt4914 and MohitGupta121 Jan 30, 2023
@rt4914
Copy link
Contributor

rt4914 commented Jan 30, 2023

@Uticodes Also update your branch.

@oppiabot oppiabot bot added the PR: LGTM label Jan 30, 2023
@Uticodes
Copy link
Contributor Author

LGTM, @Uticodes Please reply to all the earlier comments that I have left so that I can fully understand how to fixed those issues.

@rt4914 Thank you, I have dropped a reply on all the comments, kindly check.

@Uticodes
Copy link
Contributor Author

@Uticodes Also update your branch.

Done @MohitGupta121 thanks.

@Uticodes
Copy link
Contributor Author

LGTM, @Uticodes Please reply to all the earlier comments that I have left so that I can fully understand how to fixed those issues.

Done

@Uticodes
Copy link
Contributor Author

@Uticodes Also update your branch.

Done

@rt4914 rt4914 enabled auto-merge (squash) January 30, 2023 16:38
@rt4914 rt4914 merged commit 2dfad61 into oppia:develop Jan 30, 2023
@adhiamboperes adhiamboperes mentioned this pull request Apr 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MohitGupta121 MohitGupta121 MohitGupta121 approved these changes

@rt4914 rt4914 rt4914 approved these changes

Assignees

@Uticodes Uticodes

Labels
PR: LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Optimise code in profile_chooser_add_view.xml
3 participants
@Uticodes @MohitGupta121 @rt4914