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 #722: Introduced Walkthrough #723

Merged
merged 9 commits into from
Feb 28, 2020
Merged

Conversation

aggarwalpulkit596
Copy link
Contributor

Explanation

Add Walkthrough Base Classes

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.

@aggarwalpulkit596
Copy link
Contributor Author

@rt4914 before i move forward with anything else can you check whether everything is fine or not.I thought this way it would be easier for you to review the PR as well as for me make the changes

@rt4914
Copy link
Contributor

rt4914 commented Feb 27, 2020

@rt4914 before i move forward with anything else can you check whether everything is fine or not.I thought this way it would be easier for you to review the PR as well as for me make the changes

@aggarwalpulkit596 This is very awesome, you have laid out all the files perfectly. Though there are some nit changes but for now I am just focusing on the approach, which is correct. Thanks a lot.

@aggarwalpulkit596
Copy link
Contributor Author

@rt4914 just one more thing is there any custom progress bar like in the mock because I saw that progress bar in some more mocks but couldn't find it anywhere in the project

@rt4914
Copy link
Contributor

rt4914 commented Feb 27, 2020

@rt4914 just one more thing is there any custom progress bar like in the mock because I saw that progress bar in some more mocks but couldn't find it anywhere in the project

The one that you will be working on, there is nothing similar to that, you might have seen something like this in AudioPlayer but actually that is a seekbar not a progress bar. You will need to create your own progress bar for this.

@aggarwalpulkit596
Copy link
Contributor Author

@rt4914 i've completed the work can you tell about the test cases what kind of test cases should i write and for which components should i write them

@aggarwalpulkit596
Copy link
Contributor Author

Screenshot_1582868530

@aggarwalpulkit596
Copy link
Contributor Author

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.

@aggarwalpulkit596 Please do not work on code once you have assigned it for review, because it creates issues for the reviewers. If you want to work again, just unassign the reviewer first and then assign it back to you.
Currently I was reviewing your PR and in between you made commits and that brings changes to the code on which currently I am checking, which is not good.

Regarding your implementation, Next button is working only once and Back Image is not working.

Ideally what we want is that the Next should be clickable until the progress is fully finished and the Back Image should be clickable until the progress is fully empty. Also, you will need to write test cases regarding that.

app/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/walkthrough_activity.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/walkthrough_fragment.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/walkthrough_fragment.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/walkthrough_fragment.xml Outdated Show resolved Hide resolved
@rt4914
Copy link
Contributor

rt4914 commented Feb 28, 2020

@aggarwalpulkit596 Please go through this https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR to understand how Assignees work.

@aggarwalpulkit596
Copy link
Contributor Author

@aggarwalpulkit596 Please do not work on code once you have assigned it for review, because it creates issues for the reviewers. If you want to work again, just unassign the reviewer first and then assign it back to you.
Currently I was reviewing your PR and in between you made commits and that brings changes to the code on which currently I am checking, which is not good.

Regarding your implementation, Next button is working only once and Back Image is not working.

Ideally what we want is that the Next should be clickable until the progress is fully finished and the Back Image should be clickable until the progress is fully empty. Also, you will need to write test cases regarding that.

thanks for the explanation wasn't aware about the functionality and will keep that in mind for review code

@aggarwalpulkit596
Copy link
Contributor Author

@rt4914 i've made some changes please go through them once

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 very nice implementation. Thanks.

You should write more test cases to correctly verify that on next click the progress bas actually changes and on back click also.

Once this PR is submitted, we will work on second part of this PR.

Thanks.

@rt4914
Copy link
Contributor

rt4914 commented Feb 28, 2020

@aggarwalpulkit596 Always reply to these conversations, if you have solved the issue, simply write Done.

Screenshot 2020-02-28 at 1 20 06 PM

@aggarwalpulkit596
Copy link
Contributor Author

This is very nice implementation. Thanks.

You should write more test cases to correctly verify that on next click the progress bas actually changes and on back click also.

Once this PR is submitted, we will work on second part of this PR.

Thanks.

for checking progress bar progress we'll be needing a custom action similar to the link posted below
https://github.com/vanniktech/espresso-utils/blob/master/espresso-core-utils/src/main/java/com/vanniktech/espresso/core/utils/ProgressMatcher.java

@aggarwalpulkit596
Copy link
Contributor Author

@rt4914 can you help out in writing test for progress bar all I could find was this
https://stackoverflow.com/questions/57531678/assert-progress-of-progressbar-in-espresso-test
but this doesn't look good

@rt4914
Copy link
Contributor

rt4914 commented Feb 28, 2020

@aggarwalpulkit596 Everything you need regarding the progress test case is mentioned in the link shared by you: https://github.com/vanniktech/espresso-utils/blob/master/espresso-core-utils/src/main/java/com/vanniktech/espresso/core/utils/ProgressMatcher.java

Just introduce ProgressMatcher in utility package of test cases and try using that in your test case, which is mentioned here.

@rt4914 rt4914 assigned rt4914 and unassigned rt4914 Feb 28, 2020
@rt4914
Copy link
Contributor

rt4914 commented Feb 28, 2020

@aggarwalpulkit596 Have a look at it: https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section and make sure that the Assignees section points to correct person.

Also, you have haven't replied on the conversations yet. Please check that and reply accordingly.

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 think this should be the final change. Please focus on formatting related issues.

@rt4914 rt4914 assigned aggarwalpulkit596 and unassigned rt4914 Feb 28, 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, thanks.

@rt4914
Copy link
Contributor

rt4914 commented Feb 28, 2020

@aggarwalpulkit596 Once it is ready for merge, I will merge it and then you can work on #729 . Take fresh copy of develop once this PR is merged, create a new branch and then work on #729

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 everything is correct, but somehow test cases are failing on CircleCi. Will need to have a look at it.

@aggarwalpulkit596
Copy link
Contributor Author

@rt4914 test is failing for Splash screen and I haven't made any change to that

@aggarwalpulkit596
Copy link
Contributor Author

I guess there some issue with the Emulator due to which some test becomes unresponsive and hence fails

@aggarwalpulkit596
Copy link
Contributor Author

@rt4914 Can you take a look t this because I can't start the next issue as it depends upon this PR.

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 added one comment to check if circle CI passes this time. Also one more thing, merge your code with the latest develop.

@@ -119,5 +119,7 @@
<activity android:name=".testing.ReviewCardFragmentTestActivity"
android:screenOrientation="portrait"
android:theme="@style/OppiaThemeWithoutActionBar" />
<activity android:name=".walkthrough.WalkthroughActivity"
android:screenOrientation="portrait" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Just press
Control + Alt + L to reformat everything.

(This is not important but we are doing this just to make an extra commit and see if Circle CI passes.)

@rt4914 rt4914 assigned aggarwalpulkit596 and unassigned rt4914 Feb 28, 2020
@Sarthak2601
Copy link
Contributor

Hi @aggarwalpulkit596, I had a similar issue in one of my previous PRs. In my case, closing the PR and creating a new one worked. Maybe it can work for you too. :)

@aggarwalpulkit596
Copy link
Contributor Author

@Sarthak2601 instead of this we should create a better emulator which is more responsive for UI test

@rt4914
Copy link
Contributor

rt4914 commented Feb 28, 2020

Hi @aggarwalpulkit596, I had a similar issue in one of my previous PRs. In my case, closing the PR and creating a new one worked. Maybe it can work for you too. :)

Actually this issue is coming from past 1 week only and it is temporary issue it will get resolved soon. But till then we are just this so that no one gets blocked.

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

@rt4914 rt4914 merged commit 6cb7a70 into oppia:develop Feb 28, 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.

3 participants