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 #3600: Add tablet UI for FAQs and Third-party Dependencies #3671

Merged

Conversation

prayutsu
Copy link
Contributor

@prayutsu prayutsu commented Aug 12, 2021

Explanation

Fixes part of #3600:
Added new UI for tablet devices for HelpActivity that shows FAQs List and Dependencies List

Mock Link - https://xd.adobe.com/view/d405de00-a871-4f0f-73a0-f8acef30349b-a234/screen/dae1f41b-9551-430e-8b61-b285d6ee050b/

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.

Screenshots -

Portrait Landscape
Screenshot_2021-08-13-22-22-41-575_org oppia android Screenshot_2021-08-13-22-22-54-753_org oppia android
Screenshot_2021-08-13-22-22-40-037_org oppia android Screenshot_2021-08-13-22-22-53-219_org oppia android
Screenshot_1628877313 Screenshot_1628877309
Screenshot_1628877179 Screenshot_1628877269

Screenshot from 2021-08-13 23-54-07
3 test cases failing on espresso that are only meant for phones.
Screenshot from 2021-08-16 14-31-22

@oppiabot oppiabot bot unassigned rt4914 Aug 16, 2021
@oppiabot
Copy link

oppiabot bot commented Aug 16, 2021

Unassigning @rt4914 since the review is done.

@oppiabot
Copy link

oppiabot bot commented Aug 16, 2021

Hi @prayutsu, it looks like some changes were requested on this pull request by @rt4914. PTAL. Thanks!

@rt4914
Copy link
Contributor

rt4914 commented Aug 16, 2021

@Test
  fun testHelpFragment_parentIsNotExploration_checkBackArrowNotVisible() {
    launch<HelpActivity>(
      createHelpActivityIntent(
        internalProfileId = 0,
        isFromNavigationDrawer = true
      )
    ).use {
      onView(withContentDescription(R.string.abc_action_bar_up_description))
        .check(doesNotExist())
    }
  }

Screenshot from 2021-08-13 23-45-38

This test case has started failing on espresso if run on a tablet device because we now have two buttons in tablets with the same contentDescription.

We may want to change the description for one of the back buttons.
Note that we could modify the test to match the visibility to GONE but that might not be a good idea because the test case aims to verify that the back button is not present in the main toolbar when HelpActivity is loaded from Exploration.

Also, I just noticed that the back button isn't shown in mocks as there is no use of the back button on this screen until the license list or license texts are displayed.
So, I just hid the back button for these screens and modify the test cases accordingly.

Makes sense. We will need to change content description and I think we can use Navigate back to $s where s can be Third Party Dependency list or Copyright Licences depending upon which fragment is currently shown.

@prayutsu
Copy link
Contributor Author

@rt4914 The test case is now fixed -
Screenshot from 2021-08-16 14-31-22

@prayutsu prayutsu assigned rt4914 and unassigned prayutsu Aug 16, 2021
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 removed their assignment Aug 16, 2021
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @prayutsu. Had a few comments--PTAL.

@BenHenning BenHenning assigned prayutsu and unassigned BenHenning Aug 17, 2021
@prayutsu prayutsu assigned BenHenning and unassigned prayutsu Aug 17, 2021
@prayutsu prayutsu closed this Aug 17, 2021
@prayutsu prayutsu reopened this Aug 17, 2021
…let-ui-for-faq-and-third-party-dependency-lists
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @prayutsu. LGTM for codeowners.

@BenHenning
Copy link
Member

Given everyone has approved & there are no unresolved conversation threads, merging this.

@BenHenning BenHenning merged commit 0b7ce5e into develop Aug 18, 2021
@BenHenning BenHenning deleted the add-tablet-ui-for-faq-and-third-party-dependency-lists branch August 18, 2021 08:00
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