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 #4452, #4453, #4454, #4445, #4446, #4447, #4448: Add spotlight functionality #4699

Merged

Conversation

JishnuGoyal
Copy link
Contributor

@JishnuGoyal JishnuGoyal commented Nov 7, 2022

Explanation

Demo of the functionality introduced:

spotlightdemo.mp4

image

fixes #4452
fixes #4453
fixes #4454
fixes #4455
fixes #4456
fixes #4457
fixes #4458
fixes #4459

Update: The finalised design decisions for the spotlights have been resolved by this PR: #4763 (check screenshots and sample videos here) and tracked by issue #4764

This PR introduces the SpotlightFragment into the codebase, which is essentially a powerful, robust API that can highlight or 'spotlight' certain parts of the UI to create an onboarding experience for the same. We introduce the Spotlight library into the codebase handles the highlighting of certain elements on screen. The work done in this PR augments the functionality of the library, by dynamically adding arrows and hints which create an over-all Spotlight onboarding experience to the app, beautifully surfacing some functionalities of the app to a new user.

We also work on making sure that the spotlight is only shown to a user only once - the protocol buffers are used to save which spotlight has already been seen.

The purpose of this PR is to introduce an API that the future contributors can use to seamlessly integrate spotlights onto UI elements as and when needed, with minimal coding.

This PR also implements spotlights for the onboarding, home, topic and exploration screens that are required at this time.

For accessibility, the content descriptions of all the elements on which spotlights are required today are updated. If talkback is turned on, the spotlights will not show up.

The entire spotlight functionality is also gated behind a feature flag. This entire PR is backed by tests.

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

Copy link
Sponsor 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 @JishnuGoyal! Took another pass. Nearly all comments have been resolved.

I ran HomeActivityTest on Espresso, and here are the results (only testHomeActivity_configChange_promotedCard_storyNameIsCorrect is failing aside from tablet tests):

image

I also tried running SpotlightFragmentTest, but it hangs on Espresso. I didn't run any of the other suites due to my phone being very low on power.

Besides the above, I think all that's otherwise left is making sure we finalize the UI of the spotlights. Let's make sure to wrap that up before the PR is approved/merged.

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.

Approving to unblock you but one comment is pending from my earlier conversation.

@rt4914
Copy link
Contributor

rt4914 commented Nov 21, 2022

@JishnuGoyal PTAL #4699 (comment)

@BenHenning
Copy link
Sponsor Member

More Espresso results:

SpotlightFragmentTest passing:
image

PromotedStoryListViewModelTest passing:
image

PromotedStoryViewModelTest passing:
image

TopicActivityTest passing:
image

TopicPracticeFragmentTest failing (but only questions player tests which should be unrelated to the PR):
image

TopicRevisionFragmentTest:
image

Will post results for the longer tests in a subsequent comment.

@BenHenning
Copy link
Sponsor Member

OnboardingFragmentTest results (some are failing, will recheck on develop):
image

TopicFragmentTest (some failing, will recheck on develop):
image

TopicLessonsFragmentTest (some failing, will recheck on develop):
image

ExplorationActivityTest (42/57 failing, may not be able to recheck on develop since each run is 18+ mins long):
image

@BenHenning
Copy link
Sponsor Member

testOnboardingFragment_checkDefaultSlide_clickSkipButton_shiftsToLastSlide appears to be a new failure in OnboardingFragmentTest.

There are a few new potential failures in TopicFragmentTest.

35/50 tests failed in ExplorationActivityTest.

Honestly, I don't think we can really rely on these tests much, so I think more stock needs to be put into the new test suite (SpotlightFragmentTest) rather than existing test suites which may be flaky and/or already failing.

Copy link
Sponsor 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.

Couple of nits, PTAL @JishnuGoyal.

Copy link
Sponsor 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 @JishnuGoyal. PR LGTM, but please make sure to add screenshots & videos as discussed to the PR description.

@BenHenning BenHenning enabled auto-merge (squash) November 21, 2022 18:22
@oppiabot oppiabot bot added the PR: LGTM label Nov 21, 2022
@BenHenning BenHenning merged commit 642a7bd into oppia:develop Nov 22, 2022
@JishnuGoyal
Copy link
Contributor Author

Thanks, @BenHenning added a link to the screenshots and videos required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants