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 #3968: Hi fi tablet create profile reset pin fragment 2 #4011

Conversation

yash10019coder
Copy link
Contributor

@yash10019coder yash10019coder commented Nov 23, 2021

Explanation

Fixes #3968
Introduces ProfileResetPin in its own nested fragment so that it can be used in tablets to utilitize high screen density.

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 admin profile

Old Screenshot New Screenshot
Screenshot_20211201_162821 Screenshot_20211201_161133
Screenshot_20211201_162824 Screenshot_20211201_161615
Screenshot_20211201_163237 Screenshot_20211201_161933
Screenshot_20211201_163242 Screenshot_20211201_161938

For non admin profile

Old Screenshot New Screenshot
Screenshot_20211201_162836 Screenshot_20211201_161342
Screenshot_20211201_162839 Screenshot_20211201_161511
Screenshot_20211201_163256 Screenshot_20211201_162214
Screenshot_20211201_163300 Screenshot_20211201_162217

Test screenshots for ProfileResetPinActivityTest and ProfileRenamePinFragmentTest

image
image

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.

@yash10019coder PTAL. The changes suggested are almost similar to other PR.

@rt4914 rt4914 assigned yash10019coder and unassigned rt4914 and BenHenning Nov 23, 2021
Copy link
Contributor Author

@yash10019coder yash10019coder left a comment

Choose a reason for hiding this comment

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

Hi @rt4914 completed all the comments PTAL and why are so many files included in this for kdoc_test_exemptions link

Copy link
Contributor Author

@yash10019coder yash10019coder left a comment

Choose a reason for hiding this comment

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

Hi @rt4914 added all the kdocs for the settings.profile package PTAL.

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.

Nice work. Suggested nit changes.

@rt4914 rt4914 removed their assignment Dec 1, 2021
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @yash10019coder. Just a few nits left, but otherwise the PR LGTM.

yash10019coder and others added 7 commits December 16, 2021 15:05
…leEditViewModel.kt

Co-authored-by: Ben Henning <henning.benmax@gmail.com>
…leEditViewModel.kt

Co-authored-by: Ben Henning <henning.benmax@gmail.com>
…leEditViewModel.kt

Co-authored-by: Ben Henning <henning.benmax@gmail.com>
…leEditViewModel.kt

Co-authored-by: Ben Henning <henning.benmax@gmail.com>
…leResetPinViewModel.kt

Co-authored-by: Ben Henning <henning.benmax@gmail.com>
…leResetPinViewModel.kt

Co-authored-by: Ben Henning <henning.benmax@gmail.com>
Copy link
Contributor Author

@yash10019coder yash10019coder left a comment

Choose a reason for hiding this comment

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

hi @BenHenning mentioned all the comments PTAL thanks

@BenHenning BenHenning self-assigned this Dec 17, 2021
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @yash10019coder. I think you might've missed one detail in my previous comment about making sure that doc comments fill the line; I added a suggested edit to clarify. Once that's resolved, the PR will be ready for review.

@BenHenning BenHenning removed their assignment Dec 17, 2021
…leEditFragmentPresenter.kt

Co-authored-by: Ben Henning <henning.benmax@gmail.com>
Copy link
Contributor Author

@yash10019coder yash10019coder left a comment

Choose a reason for hiding this comment

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

Hi @BenHenning commited the suggested changes PTAL thanks.

@yash10019coder
Copy link
Contributor Author

Thanks @yash10019coder. I think you might've missed one detail in my previous comment about making sure that doc comments fill the line; I added a suggested edit to clarify. Once that's resolved, the PR will be ready for review.

done

@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.

Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @yash10019coder. LGTM for codeowners & doc changes.

@oppiabot
Copy link

oppiabot bot commented Dec 17, 2021

Unassigning @BenHenning since they have already approved the PR.

@oppiabot oppiabot bot added the PR: LGTM label Dec 17, 2021
@BenHenning BenHenning merged commit 53e2321 into oppia:develop Dec 17, 2021
@yash10019coder yash10019coder mentioned this pull request Jun 14, 2022
4 tasks
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.

Introduce ProfileResetPinFragment
3 participants