-
Notifications
You must be signed in to change notification settings - Fork 21
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 isLoading State to Chat #1018
Conversation
platforms/intellij/compose/src/jvmMain/kotlin/foundry/intellij/compose/aibot/ChatWindowUi.kt
Outdated
Show resolved
Hide resolved
spacing: Dp = 7.dp, | ||
movementDistance: Dp = 10.dp, | ||
) { | ||
val animatedDots = List(4) { remember { Animatable(0f) } } |
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.
This will re-allocated every time, shouldn't the remember be outside of the List
creation?
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.
Yeah, you are right. The remember should be outside the List block because otherwise it would be calling remember on each item of the list. Thank you for catching that, I made the change here: 3ca3039
platforms/intellij/compose/src/jvmMain/kotlin/foundry/intellij/compose/aibot/ChatPresenter.kt
Outdated
Show resolved
Hide resolved
…/add_loading_state
…e-plugin into kl/add_loading_state
// tutorial from https://www.youtube.com/watch?v=xakNOVaYLAg | ||
|
||
@Composable |
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.
nit: let's make this a doc on the function rather than this floating comment, that way it's attached in kdoc
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.
Is this okay? 843a85b
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.
Yeah but let's add a proper description to it too, make the source just a footnote at the end for reference
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.
Does this description look good? 23bf68d
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 quite. The kdoc should say what the composable itself does, not just where it's from. Let's do both
/**
* This shows a loading indicator of three bouncing dots, etc etc.
*
* @see <a href="https://...">Source</a>
*/
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.
Sounds good, thanks! d6d7098
I made the adjustment here.
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.
Adjusted here: b457fec
} | ||
} | ||
|
||
val animatedValues = animatedDots.map { it.value } |
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.
This will reallocate every time too. Do you maybe want a SnapshotStateList instead?
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.
Sounds good, so does It prevent the recreation of our Animatable objects on each recomposition? I implemented a change here: 23bf68d
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.
Actually - do we need this intermediate variable at all? i.e. why not just do animatedDots.forEach { dot ->
in the below block?
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.
Oh yeah that's a great point. I can use dot.value instead too
@@ -131,13 +139,13 @@ private fun ConversationField(modifier: Modifier = Modifier, onSendMessage: (Str | |||
Column(Modifier.fillMaxHeight(), verticalArrangement = Arrangement.Center) { | |||
// button will be disabled if there is no text | |||
IconButton( | |||
modifier = Modifier.padding(4.dp).enabled(isTextNotEmpty), | |||
modifier = Modifier.padding(4.dp).enabled(isTextNotEmpty && !isLoading), |
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.
IconButton
has an enabled
parameter we can use, any reason not to use that? That should handle internal state better too
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 did it mainly for how it looks. The enabled parameter doesn't make the button less faded, so I wanted to make it more obvious it's disabled when I set it.
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.
interesting. Ok let's keep this but file an issue on jewel about making the button disabled appearance reflect it, feels like a bug that it doesn'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.
One last question, also link the issue you file on the jewel repo for posterity 👍
* Adopted from the Three-Dot Loading Animation Tutorial with Jetpack Compose by Stevdza-San from | ||
* | ||
* @see <a href="https://www.youtube.com/watch?v=xakNOVaYLAg">Source</a> |
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.
Nit before you merge: you can just use the @see
tag, no need for both
} | ||
} | ||
|
||
val animatedValues = animatedDots.map { it.value } |
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.
Actually - do we need this intermediate variable at all? i.e. why not just do animatedDots.forEach { dot ->
in the below block?
Adding a state for if it's loading once the API is called, so that a loading animation can be added while waiting for the chat response.
Below is an example of the animation. I enabled it so it's always loading just to show the animation (since the API call hasn't been merged yet)
https://github.com/user-attachments/assets/7abb3041-7aa0-4117-94e6-710a6b0cb800
I also filed an issue with the appearance of the enabled parameter of the IconButton here: JetBrains/jewel#633