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 part of #1587: Made intent keys consistent in the options and player packages in the app module #2066

Merged

Conversation

janewaitara
Copy link
Contributor

@janewaitara janewaitara commented Oct 30, 2020

Explanation

Fixes part of #1587 Made intent keys consistent in the options and player packages in the app module

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.

@janewaitara
Copy link
Contributor Author

@rt4914, PTAL

@anandwana001
Copy link
Contributor

@janewaitara Thanks for the PR.

Few points to follow before review:

  1. Follow the first point from the 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: ...".)
  2. Follow on how to choose a branch name from our wiki - https://github.com/oppia/oppia-android/wiki#instructions-for-making-a-code-change
  3. As we know to fix this issue, we need a lot of changes, so how are we planning with the sub PR's? Are we going to have sub PR's package way? Like fixing 2-3 packages at a time?
  4. As per my point 3, we are fixing a part of an issue, so please follow the second point from the checklist The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).

@janewaitara janewaitara changed the title Fix #1587 Made intent keys consistent in the options and player packages in the app module Fix part of #1587 Made intent keys consistent in the options and player packages in the app module Oct 31, 2020
@janewaitara
Copy link
Contributor Author

@anandwana001 Thank you for the corrections.

I have worked on point 1 and 4 for the PR title and description. For the intent keys, tags and bundle keys, I opted to work on files in the same package and have the sub PRs in the package ways since the files would be too many in one PR. However, for the saved instances, the files were few hence I worked on them all in all packages.

@janewaitara janewaitara changed the title Fix part of #1587 Made intent keys consistent in the options and player packages in the app module Fix part of #1587: Made intent keys consistent in the options and player packages in the app module Oct 31, 2020
Copy link
Contributor

@anandwana001 anandwana001 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 @janewaitara
Defer to @rt4914 for further review

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.

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.

Blocking this PR just so that @janewaitara can update the branch and merge with latest develop.

@rt4914 rt4914 removed their assignment Nov 25, 2020
@janewaitara
Copy link
Contributor Author

@rt4914 I have updated the branch and merged with the latest develop. 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.

LGTM, thanks.

@rt4914 rt4914 merged commit ebf58a8 into oppia:develop Nov 25, 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.

None yet

3 participants