-
Notifications
You must be signed in to change notification settings - Fork 936
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
introducing confetti animation using konfetti library #1743
Conversation
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.
Just a superficial look at the code for now.
@@ -100,5 +102,11 @@ class NumberDialog : AppCompatDialogFragment() { | |||
val notes = view.notes.text.toString() | |||
onToggle(value, notes) | |||
requireDialog().dismiss() | |||
val v = requireActivity().findViewById<LinearLayout>(R.id.konfettiLayout) |
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.
v
is called konfettiView
above, consistency?
@@ -100,5 +102,11 @@ class NumberDialog : AppCompatDialogFragment() { | |||
val notes = view.notes.text.toString() | |||
onToggle(value, notes) | |||
requireDialog().dismiss() | |||
val v = requireActivity().findViewById<LinearLayout>(R.id.konfettiLayout) | |||
|
|||
if (value > 0.0) { |
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 it should be on completion rather than for any value? Not sure.
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.
added a code comment.
Reason: I thought it would be motivating for users to at least add some value rather than not doing the habit. 😸
@@ -88,25 +89,28 @@ class CheckmarkButtonView( | |||
setOnLongClickListener(this) | |||
} | |||
|
|||
fun performToggle() { | |||
fun performToggle(v: View) { |
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.
Same remark about this v
variable, make it more explicit so that it's easier to understand.
} | ||
|
||
fun showConfetti(view: View) { | ||
val viewId = R.id.konfetttiView |
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.
val viewId = R.id.konfetttiView | |
val viewId = R.id.konfettiView |
Is this a typo or is there a reason to have 3 t
?
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.
not typo, i was just using 3 t
to differentiate the variables in kt files and in XML. Just a minor naming convention
<nl.dionsegijn.konfetti.xml.KonfettiView | ||
android:layout_width="match_parent" | ||
android:layout_height="match_parent" | ||
android:id="@+id/konfetttiView"/> |
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.
android:id="@+id/konfetttiView"/> | |
android:id="@+id/konfettiView"/> |
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.
fixed few comments, fixed conflicts and responded to some comments
@@ -100,5 +102,11 @@ class NumberDialog : AppCompatDialogFragment() { | |||
val notes = view.notes.text.toString() | |||
onToggle(value, notes) | |||
requireDialog().dismiss() | |||
val v = requireActivity().findViewById<LinearLayout>(R.id.konfettiLayout) | |||
|
|||
if (value > 0.0) { |
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.
added a code comment.
Reason: I thought it would be motivating for users to at least add some value rather than not doing the habit. 😸
} | ||
|
||
fun showConfetti(view: View) { | ||
val viewId = R.id.konfetttiView |
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.
not typo, i was just using 3 t
to differentiate the variables in kt files and in XML. Just a minor naming convention
I don't want to tell you to do 1. yet because I'm still not sure @iSoron is open to this new UI feature, better to wait for a confirmation first. @iSoron can you give us your opinion (e.g. based on the video) before @gokulk16 continues working on this? |
Just wanted to chip in that the lack of a strong blue colour (e.g. 0000FF) in that confetti would make me not use it. It's not confetti without all primary colours. (Only half-joking) |
Thank you for the PR, @gokulk16. I have merged it to dev (06090e2) with the following major changes:
Feel free to submit other PRs to tweak it further or fix any remaining issues. At this point, I think the animation is subtle enough for it to be enable by default. If users complain about it, we can add an option in the settings to disable it, or even make it opt-in. |
@iSoron Thanks for the tweaks and also merging the PR. 🙏 |
Test cases when confetti will be shown for habit type - boolean(Yes/No):
Test cases when confetti will be shown for habit type - measurable:
Settings combinations tested with:
Screen recording of confetti animation:
Confetti.recording.in.nexus.4.mp4