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 #678 : Landscape onBoarding workflow #698

Merged
merged 6 commits into from
Feb 24, 2020
Merged

Fix #678 : Landscape onBoarding workflow #698

merged 6 commits into from
Feb 24, 2020

Conversation

Sarthak2601
Copy link
Contributor

@Sarthak2601 Sarthak2601 commented Feb 22, 2020

Explanation

  • Fixes part of Landscape Lowif: Onboarding Flow #678
  • Added the svg files for the landscape view
  • One of the svg file contained an image link so replaced that with path source and everything to make that image come back
  • Handled the source of the slide image using the orientation configuration of android in OnboadingSlideViewModel.kt
  • Changed the ScreenOrientation of the onBoarding activity in the Manifest

Screenshots -

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.

@Sarthak2601
Copy link
Contributor Author

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

The initial structure looks correct but if you look at the mock correctly, the left side of screen contains the image background.

Mock:
Screenshot 2020-02-23 at 11 45 22 AM

App:
Screenshot 2020-02-23 at 11 45 09 AM

@Sarthak2601
Copy link
Contributor Author

The initial structure looks correct but if you look at the mock correctly, the left side of screen contains the image background.

Mock:
Screenshot 2020-02-23 at 11 45 22 AM

App:
Screenshot 2020-02-23 at 11 45 09 AM

Actually, I wanted to discuss this with you. The thing here is that we have the constraint layout with the skip button and the dots at the bottom in the fragment and the slide gets loaded in the view pager above that. As the slides load in the view pager it can't be continued till the end because of the presence of the another constraint layout.

@PrarabdhGarg
Copy link
Contributor

The initial structure looks correct but if you look at the mock correctly, the left side of screen contains the image background.
Mock:
Screenshot 2020-02-23 at 11 45 22 AM
App:
Screenshot 2020-02-23 at 11 45 09 AM

Actually, I wanted to discuss this with you. The thing here is that we have the constraint layout with the skip button and the dots at the bottom in the fragment and the slide gets loaded in the view pager above that. As the slides load in the view pager it can't be continued till the end because of the presence of the another constraint layout.

@Sarthak2601 what you could do for that is that instead of constraining the bottom of the viewpager with the top of the bottom container, you could constraint it with the bottle of the parent. That would give it the necessary overlap required.

@Sarthak2601
Copy link
Contributor Author

The initial structure looks correct but if you look at the mock correctly, the left side of screen contains the image background.
Mock:
Screenshot 2020-02-23 at 11 45 22 AM
App:
Screenshot 2020-02-23 at 11 45 09 AM

Actually, I wanted to discuss this with you. The thing here is that we have the constraint layout with the skip button and the dots at the bottom in the fragment and the slide gets loaded in the view pager above that. As the slides load in the view pager it can't be continued till the end because of the presence of the another constraint layout.

@Sarthak2601 what you could do for that is that instead of constraining the bottom of the viewpager with the top of the bottom container, you could constraint it with the bottle of the parent. That would give it the necessary overlap required.

Oh yes, found that. Thank you so much @PrarabdhGarg :-)

@Sarthak2601
Copy link
Contributor Author

Updated the positioning of the get started button in accordance with the mock.

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

@Sarthak2601 Looks good, thanks.
Update the title of this PR: Fix #678: Landscape onBoarding workflow

Also, now we can focus on test cases.

Check OnboardingFragmentTest in this you will find a lot of test cases for portrait mode. In this file you will need add following test cases for orientation mode.

  1. Rotate to orientation mode, check slide_0 title, if this is correct or not.
  2. In portrait mode, move to slide_1 , change to landscape mode and now check if the title is correct or not.
  3. In portrait mode, click on skip, change to landscape mode and now check the title is same as last slide title or not.

Regarding how to change to orientation landscape in a test case, you can see test cases in AdminAuthActivityTest

@Sarthak2601
Copy link
Contributor Author

@Sarthak2601 Looks good, thanks.
Update the title of this PR: Fix #678: Landscape onBoarding workflow

Also, now we can focus on test cases.

Check OnboardingFragmentTest in this you will find a lot of test cases for portrait mode. In this file you will need add following test cases for orientation mode.

  1. Rotate to orientation mode, check slide_0 title, if this is correct or not.
  2. In portrait mode, move to slide_1 , change to landscape mode and now check if the title is correct or not.
  3. In portrait mode, click on skip, change to landscape mode and now check the title is same as last slide title or not.

Regarding how to change to orientation landscape in a test case, you can see test cases in AdminAuthActivityTest

Okay, I will work on it 👍
Thank you so much for the guidance :-)

@Sarthak2601
Copy link
Contributor Author

The initial structure looks correct but if you look at the mock correctly, the left side of screen contains the image background.

Mock:
Screenshot 2020-02-23 at 11 45 22 AM

App:
Screenshot 2020-02-23 at 11 45 09 AM

Done 👍

@Sarthak2601 Sarthak2601 changed the title Fixes part of #678 : Landscape onBoarding workflow Fix #678 : Landscape onBoarding workflow Feb 24, 2020
@Sarthak2601
Copy link
Contributor Author

@Sarthak2601 Looks good, thanks.
Update the title of this PR: Fix #678: Landscape onBoarding workflow

Also, now we can focus on test cases.

Check OnboardingFragmentTest in this you will find a lot of test cases for portrait mode. In this file you will need add following test cases for orientation mode.

  1. Rotate to orientation mode, check slide_0 title, if this is correct or not.
  2. In portrait mode, move to slide_1 , change to landscape mode and now check if the title is correct or not.
  3. In portrait mode, click on skip, change to landscape mode and now check the title is same as last slide title or not.

Regarding how to change to orientation landscape in a test case, you can see test cases in AdminAuthActivityTest

-Added test cases
-Changed the PR title

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

Some small changes suggested.

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.

These 3 test cases are failing.
Update them as following:

  @Test
  fun testOnboardingFragment_slide0title_inlandscape_iscorrectornot() {
    launch(OnboardingActivity::class.java).use {
      onView(isRoot()).perform(orientationLandscape())
      onView(
        allOf(
          withId(R.id.slide_title_text_view),
          isCompletelyDisplayed()
        )
      ).check(matches(withText(R.string.onboarding_slide_0_title)))
    }
  }

  @Test
  fun testOnboardingFragment_movetoslide1_changeorientation_checktitle() {
    launch(OnboardingActivity::class.java).use {
      onView(withId(R.id.onboarding_slide_view_pager)).perform(scrollToPage(1))
      onView(isRoot()).perform(orientationLandscape())
      onView(
        allOf(
          withId(R.id.slide_title_text_view),
          isCompletelyDisplayed()
        )
      ).check(matches(withText(R.string.onboarding_slide_1_title)))
    }
  }

  @Test
  fun testOnboardingFragment_clickonskip_changeorientation_checktitle() {
    launch(OnboardingActivity::class.java).use {
      onView(withId(R.id.skip_text_view)).perform(click())
      onView(isRoot()).perform(orientationLandscape())
      onView(
        allOf(
          withId(R.id.slide_title_text_view),
          isCompletelyDisplayed()
        )
      ).check(matches(withText(R.string.onboarding_slide_3_title)))
    }
  }

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.

@Sarthak2601
Copy link
Contributor Author

LGTM. Thanks.

Thank you so much !
You helped me a lot in this :-)

@rt4914
Copy link
Contributor

rt4914 commented Feb 24, 2020

LGTM. Thanks.

Thank you so much !
You helped me a lot in this :-)

No worries. I will mostly assign you some new tasks to work on tomorrow.

@Sarthak2601
Copy link
Contributor Author

Sarthak2601 commented Feb 24, 2020

LGTM. Thanks.

Thank you so much !
You helped me a lot in this :-)

No worries. I will mostly assign you some new tasks to work on tomorrow.

Sounds great !

@rt4914 rt4914 merged commit 3f0ba97 into oppia:develop Feb 24, 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