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 #692: Lowfi: HelpActivity (Part 1) #724

Merged
merged 37 commits into from
Mar 7, 2020
Merged

Fix #692: Lowfi: HelpActivity (Part 1) #724

merged 37 commits into from
Mar 7, 2020

Conversation

SayantanBanerjee16
Copy link
Contributor

@SayantanBanerjee16 SayantanBanerjee16 commented Feb 28, 2020

Explanation

Fixes #692:
The recycler view has been implemented through MVVM and its working fine, FAQ activity, fragment & their presenters are implemented and also tests have been written.

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.

I have just taken an overview look now. You will mainly need to change the implementation of Recyclerview.

app/build.gradle Outdated Show resolved Hide resolved
app/build.gradle Outdated Show resolved Hide resolved
app/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved
app/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved
app/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/help/HelpActivity.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/help/HelpItemListener.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/help/HelpViewModel.kt Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned SayantanBanerjee16 and unassigned rt4914 Feb 28, 2020
@SayantanBanerjee16
Copy link
Contributor Author

Thanks for the review. I will do the changes accordingly.

@SayantanBanerjee16
Copy link
Contributor Author

SayantanBanerjee16 commented Feb 29, 2020

@rt4914 Done with all the changes you had suggested. Also Recycler View is working fine along with clicking on the FAQ item, FAQ activity is launched. Also, the relevant Test implemented.

Please review. Thanks :)

@SayantanBanerjee16 SayantanBanerjee16 changed the title Fix part of #692: Lowfi: HelpActivity (Part 1) Fix #692: Lowfi: HelpActivity (Part 1) Mar 1, 2020
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.

I have mentioned some implementation change to layout options in HelpActivity. PTAL

app/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/help/HelpFragment.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/help/HelpViewModel.kt Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned SayantanBanerjee16 and unassigned rt4914 Mar 2, 2020
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.

I have suggested changes. In short to improvise on your PR quality you can following following steps:

  1. Once your PR is ready, go to Files Changed.
  2. Check if there are any unnecessary files changes which you have not introduced but are still showing because of android studio, then revert those changes.
  3. Now go through each file in Files Changed and open that same file in Android Studio and do following steps:
  • Control + Alt + L - formatting shortcut.
  • Try to solve all possible warnings given by android studio.
  1. Once all these changes are done, run the test cases which you have written in this PR, if any.

Once all this is done, commit again if there are any changes and then assign it to the reviewers.

app/src/main/java/org/oppia/app/help/HelpActivity.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/help/HelpViewModel.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/help/RoutetoFAQ.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/help/faq/FAQActivity.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/help/faq/FAQActivity.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/help/faq/FAQActivity.kt Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned SayantanBanerjee16 and unassigned rt4914 Mar 5, 2020
@SayantanBanerjee16
Copy link
Contributor Author

@rt4914 OnCLick has been handled in Adapter instead of View Model and also every other NIT fixed, indentation fix is done, and removed any unused lines, calls, and imports. Please 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.

This looks like a very nice implementation. Just some final changes suggested. Thanks.

@rt4914 rt4914 assigned SayantanBanerjee16 and unassigned rt4914 Mar 6, 2020
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, there are some nit changes PTAL.

Also, I think after these nit changes we can merge this PR.

@rt4914
Copy link
Contributor

rt4914 commented Mar 6, 2020

@SayantanBanerjee16 There is one suggestion, do not mark conversations Resolve conversation directly, instead, reply to the conversation, something like Done and when I review it again, I will mark it as resolve that way its easier to review PRs.

@SayantanBanerjee16
Copy link
Contributor Author

@SayantanBanerjee16 There is one suggestion, do not mark conversations Resolve conversation directly, instead, reply to the conversation, something like Done and when I review it again, I will mark it as resolve that way its easier to review PRs.

Okay, will follow this from next time.

@SayantanBanerjee16
Copy link
Contributor Author

@rt4914 Done the NIT changes. Please merge if it's good to go. Thanks :)

@rt4914 rt4914 merged commit 0e319b3 into oppia:develop Mar 7, 2020
@SayantanBanerjee16 SayantanBanerjee16 deleted the help-activity branch March 7, 2020 04:01
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.

Lowfi: HelpActivity (Part 1)
2 participants