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 #4494: Hint bulb animation added #4501

Merged
merged 6 commits into from
Aug 19, 2022
Merged

Conversation

rt4914
Copy link
Contributor

@rt4914 rt4914 commented Aug 15, 2022

Explanation

Fixes #4494

This PR introduces a bouncing animation for the light bulb to make it easier to notice. It has an animation delay of 5 seconds and will repeat every 30 seconds. The animation itself lasts about 6 seconds (see the video below). The animation will continue until the user reveals the hint or moves on to the next card.

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).

For UI-specific PRs only

Demonstration video:

hint_bulb_bouncing.mp4

@rt4914
Copy link
Contributor Author

rt4914 commented Aug 16, 2022

In this PR Bazel related CI checks are failing which I am unable to solve fully right now.

  1. I think I need to mention the newly added hint_bulb_animation.xml file somewhere in bazel files for correct build but I am unable to find the correct file.
  2. There is some CI check because of TODO check but I am finding it weird as why this check is failing because it seems it should not be related to this PR.

@rt4914
Copy link
Contributor Author

rt4914 commented Aug 16, 2022

In this PR Bazel related CI checks are failing which I am unable to solve fully right now.

  1. I think I need to mention the newly added hint_bulb_animation.xml file somewhere in bazel files for correct build but I am unable to find the correct file.
  2. There is some CI check because of TODO check but I am finding it weird as why this check is failing because it seems it should not be related to this PR.

All CI checks are passing now.

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 @rt4914! The video seems a bit like the animation is intermittent, but it might just be the recording and/or my computer. I'll check it out locally to see how it looks on a device.

@@ -338,7 +341,7 @@ class StateFragmentPresenter @Inject constructor(
oppiaLogger.e("StateFragment", "Failed to retrieve hint/solution", result.error)
} else {
// If the hint/solution, was revealed remove dot and radar.
viewModel.setHintOpenedAndUnRevealedVisibility(false)
setHintOpenedAndUnRevealed(false)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Reminders to add tests & add this to questions' implementation, too.

app/src/main/res/anim/hint_bulb_animation.xml Outdated Show resolved Hide resolved
@seanlip
Copy link
Member

seanlip commented Aug 18, 2022

Just a note -- agreed with @BenHenning that the pulsing animation looks a bit weird. It looked better in the old video.

The behaviour of "flash 5 seconds, stop 25 seconds" seems fine to me though! Thanks Rajat :)

@seanlip seanlip removed their assignment Aug 18, 2022
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.

Approving this with some open comments. #4510 is tracking addressing those comments post-merge. Plus, an additional review should take place for the changes I contributed directly to the PR (since they haven't yet been reviewed).

@BenHenning BenHenning merged commit b569498 into develop Aug 19, 2022
@BenHenning BenHenning deleted the hint-bulb-animation branch August 19, 2022 09:26
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.

Make the hint button animate.
3 participants