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

Fixes part of #1267: Added newInstance function in TopicInfoFragment #1280

Merged
merged 3 commits into from
Jun 11, 2020

Conversation

sajalasati
Copy link
Contributor

@sajalasati sajalasati commented Jun 9, 2020

Explanation

Partly fixes #1267.
I have added a newInstance function in the TopicInfoFragment file as part of the code optimization suggested in the issue.
I request @vinitamurthi @nikitamarysolomanpvt @rt4914 to please this review this.

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

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.

This is nice but the function is not getting used anywhere, left some comments accodingly.

@rt4914 rt4914 assigned sajalasati and unassigned rt4914 Jun 9, 2020
@nikitamarysolomanpvt nikitamarysolomanpvt removed their assignment Jun 9, 2020
@sajalasati sajalasati requested a review from rt4914 June 9, 2020 16:21
Copy link
Contributor

@vinitamurthi vinitamurthi left a comment

Choose a reason for hiding this comment

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

LGTM, I will defer to @rt4914 and @nikitamarysolomanpvt for the structure of this

@rt4914
Copy link
Contributor

rt4914 commented Jun 10, 2020

@sajalasati The Title of the PR should be Fixes part of #1267: Added newInstance function in TopicInfoFragment

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.

@sajalasati sajalasati changed the title Fixes part of #1267, added new instance function in TopicInfoFragment Fixes part of #1267: Added newInstance function in TopicInfoFragment Jun 10, 2020
Copy link
Contributor

@nikitamarysolomanpvt nikitamarysolomanpvt left a comment

Choose a reason for hiding this comment

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

LGTM, besides the naming of argument key for example PROFILE_ID_ARGUMENT_KEY. Hope this will be handled in another PR.

@vinitamurthi
Copy link
Contributor

Looks like tests pass so I am merging this PR now

@vinitamurthi vinitamurthi merged commit 1117cb6 into oppia:develop Jun 11, 2020
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.

Implementation change of fragment new instance
5 participants