-
Notifications
You must be signed in to change notification settings - Fork 206
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
Add achivement click animation #1235 #1265
base: main
Are you sure you want to change the base?
Add achivement click animation #1235 #1265
Conversation
) | ||
|
||
sealed interface ClickedAchievementState { |
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.
How about naming this AchievementAnimationState and AchievementAnimationState.Animating AchievementAnimationState.NotAnimating?
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.
The namings look better. I will rename them.
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.
I would like to have a screenshot for this ideally. Could you see it? Maybe we can add a archivement by injecting the datastore in the test. 👀
showAnimation = { achievement -> viewModel.onClickAchievement(achievement) }, | ||
finishAnimation = viewModel::onFinishAnimation, |
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.
We would like to promote the best practice that a StateHolder should be notified events.
So how about naming this onAchivementClicked
and judge if we run a animation in ViewModel for showAnimation
?
And use "onAnimationFinished" as a name for finishAnimation
?
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.
Renamed showAnimation to onAchivementClick and finishAnimation to onAnimationFinish.
@@ -120,13 +121,17 @@ class AchievementsScreenViewModel @Inject constructor( | |||
) | |||
} | |||
|
|||
private val clickedAchievement = MutableStateFlow<AchievementAnimationState>(AchievementAnimationState.NotAnimating) |
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.
Could you change this name as we change the state? 🙇
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.
Sorry for overlooking. I renamed clickedAchievement to achievementAnimationState.
I've tried to add the screenshot tests. It seems to work fine. |
@@ -30,6 +38,11 @@ class AchievementsScreenTest { | |||
@Inject | |||
lateinit var achievementsScreenRobot: AchievementsScreenRobot | |||
|
|||
@Before | |||
fun setup() { | |||
LottieTask.EXECUTOR = Executor(Runnable::run) |
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.
Maybe you should restore the executor after test to isolate the tests 👀
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.
@takahirom
I've checked if this setup affects other test classes.
To invesigate that, I created AchievementsScreenTest2 copying original one except setUp function. As a result, all the test cases in AchievementsScreenTest2 didn't capture achivement highlight correctly because LottieTask.EXECUTOR was not set up for tests.
So, I assume changing LottieTask.EXECUTOR is confined in its test class and tearDown isn't really necessary.
However, if you are concerned about other test cases in the same test class, which doesn't need to set up LottieTask.EXECUTOR, I'd make setup in each test cases like below. But I feel it's kind of hacky.
fun checkHighlightImageA() {
achievementsScreenRobot {
LottieTask.EXECUTOR = Executor(Runnable::run)
setupSavedAchievement(ArcticFox)
setupScreenContent()
clickAchievementImageA()
checkScreenCapture()
LottieTask.EXECUTOR = Executors.newCachedThreadPool()
}
}
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.
As far as I know static variable is used across the tests. We have some options to do it.
How about adding it here?
Line 64 in b530571
.around(object : TestWatcher() { |
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.
@takahirom
I wasn't aware of that system.
I added LottieTestRule.
Issue
Overview (Required)
Movie (Optional)
achievement_animation.mp4