-
Notifications
You must be signed in to change notification settings - Fork 529
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 #4765: The Continue animation isn't interactive #4814
Conversation
Thanks for submitting this pull request! Some main reviewers |
Hi @JishnuGoyal, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @JishnuGoyal. Left one follow-up comment. Will follow up on the animation specifically in a follow-up comment.
app/src/main/java/org/oppia/android/app/customview/ContinueButtonView.kt
Outdated
Show resolved
Hide resolved
So I definitely think the wobble is better than the pulsing. It still has a disadvantage in that the touch target is moving, but it's much less likely to introduce clicking issues here than with the previous animation. I'm less certain about the 8 second delay between the animations--that seems a bit long. @seanlip do you have any thoughts on the updated animation? |
I think the updated animation looks fine -- let's go with it. I think the 8-second delay is OK since this isn't meant to be as "in your face". |
PTAL @BenHenning. Going with the current animation per the suggestion given by Sean. |
Unassigning @JishnuGoyal since a re-review was requested. @JishnuGoyal, please make sure you have addressed all review comments. Thanks! |
Hi @JishnuGoyal, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @seanlip and @JishnuGoyal.
@JishnuGoyal the PR LGTM. There's one comment for you to resolve (I'm fine with you marking it as resolved once it's addressed one way or the other), and I think the PR needs to be updated with the latest develop before it can be merged. You'll also need @rt4914's approval for the XML changes.
app/src/main/java/org/oppia/android/app/customview/ContinueButtonView.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice and elegant solution. Thanks @JishnuGoyal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meant to approve it.
Thank you so much @rt4914! Glad to hear this! <3 |
Thanks @JishnuGoyal. Sent a follow-up comment--happy to defer to your decision here. |
Thanks Ben! I have left a reply on the last review thread. I suggest going with the current implementation itself since the function signature doesn't seem to match. PTAL @BenHenning. |
Unassigning @JishnuGoyal since a re-review was requested. @JishnuGoyal, please make sure you have addressed all review comments. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @JishnuGoyal. LGTM.
app/src/main/java/org/oppia/android/app/customview/ContinueButtonView.kt
Outdated
Show resolved
Hide resolved
<!-- READ ME FIRST: Please fill in the explanation section below and check off every point from the Essential Checklist! --> ## Explanation Fixes oppia#4765 Demo Video: https://user-images.githubusercontent.com/64526117/210055420-016f2dd2-46d6-4166-ae2a-be4c98710299.mp4 This PR tries to handle the problem of the continue interaction button's animation not being eye-catching to the users. Currently, it does so by changing the animation to a shake animation. The button now does a little wobble, starting 45s after the screen is shown. It wobbles 2 times, and then repeats the animation after every 8 seconds. Only with further testing and trial, will we know how the animation appeals to the users and resolves the problem at hand. **Technical detail:** The anim file (xml) is set to play the animation 2 times. In the `ContinueButtonView.kt` file, the startAnimating() function has been made recursive, which calls itself after every 8 seconds in order to repeat the animation. The 8 second gap is achieved using the lifeCycleSafeTimerFactory. ## Essential Checklist <!-- Please tick the relevant boxes by putting an "x" in them. --> - [x] 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: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only
Explanation
Fixes #4765
Demo Video:
Screenrecording_20221229_231926.mp4
This PR tries to handle the problem of the continue interaction button's animation not being eye-catching to the users. Currently, it does so by changing the animation to a shake animation. The button now does a little wobble, starting 45s after the screen is shown. It wobbles 2 times, and then repeats the animation after every 8 seconds.
Only with further testing and trial, will we know how the animation appeals to the users and resolves the problem at hand.
Technical detail:
The anim file (xml) is set to play the animation 2 times.
In the
ContinueButtonView.kt
file, the startAnimating() function has been made recursive, which calls itself after every 8 seconds in order to repeat the animation. The 8 second gap is achieved using the lifeCycleSafeTimerFactory.Essential Checklist
For UI-specific PRs only