Add Circuit logic to AI Bot #955
Conversation
skate-plugin/project-gen/src/jvmMain/kotlin/slack/tooling/aibot/ChatColors.kt
Outdated
Show resolved
Hide resolved
| val circuit = | ||
| Circuit.Builder() | ||
| .addPresenter<ChatScreen, ChatScreen.State>(ChatPresenter()) | ||
| .addUi<ChatScreen, ChatScreen.State> { state, modifier -> ChatWindow(state, modifier) } | ||
| .build() |
There was a problem hiding this comment.
you should wrap this in a remember { } block, otherwise it'll get recreated every time
There was a problem hiding this comment.
f27ddc1 under ChatPanel.kt
| val newMessage = Message(event.message, isMe = true) | ||
| messages = messages + newMessage | ||
| val response = Message(callApi(event.message), isMe = false) | ||
| messages = messages + response | ||
| currentInput = "" // clear chat after sending |
There was a problem hiding this comment.
I know this is just stubbed out, but for future ref you don't wanna have too much logic in this even callback directly. Instead, you generally want it to just update internal state.
| val newMessage = Message(event.message, isMe = true) | ||
| messages = messages + newMessage | ||
| val response = Message(callApi(event.message), isMe = false) | ||
| messages = messages + response | ||
| currentInput = "" // clear chat after sending |
There was a problem hiding this comment.
I would suggest keeping "currentInput" entirely in the UI, and just use this event to signal a new sent message with the text received from the UI. What do you think?
There was a problem hiding this comment.
Yeah this makes sense, I should just keep it in UI. Will make this change! Thanks
There was a problem hiding this comment.
I removed it in f27ddc1 under ChatPresenter.kt
| sealed class Event : CircuitUiEvent { | ||
| data class SendMessage(val message: String) : Event() | ||
|
|
||
| data class UpdateInput(val input: String) : Event() |
There was a problem hiding this comment.
I thought since SendMessage is an event, then I should be using CircuitUiEvent with a sealed class. That's how I set it up for the ChatPresenter. But after I removed currentInput from the presenter, I don't need that UpdateInput class.
| object ChatWindowUi { | ||
| @Composable | ||
| fun ChatWindow(state: ChatScreen.State, modifier: Modifier = Modifier) { |
There was a problem hiding this comment.
Generally we just keep these functions top-level, no need for the wrapper object
There was a problem hiding this comment.
f27ddc1 in ChatWindowUi.kt
| } | ||
|
|
||
| @Composable | ||
| fun ConversationField(modifier: Modifier = Modifier, onSendMessage: (String) -> Unit) { |
There was a problem hiding this comment.
nit: private should be the default
|
|
||
| import androidx.compose.runtime.Immutable | ||
|
|
||
| // not sure if I need this in a separate class |
There was a problem hiding this comment.
Could you expand on this?
There was a problem hiding this comment.
Do you think it's okay to have the Message data class in a separate file? Or is it unnecessary?
There was a problem hiding this comment.
if you're going to reuse it yeah, otherwise you could just co-locate it with the rest of your state models
| keyboardActions = KeyboardActions.Default, | ||
| maxLines = Int.MAX_VALUE, |
There was a problem hiding this comment.
Did you mean to leave these defaults in?
There was a problem hiding this comment.
I did initially because I was trying different things for the "shift+enter" functionality. I just realized that it doesn't make a difference, so I will remove it. Thanks for pointing it out!
| Icon( | ||
| painter = painterResource("/drawable/send.svg"), | ||
| contentDescription = "Send", | ||
| modifier = Modifier.size(20.dp), |
There was a problem hiding this comment.
For this I would recommend using Modifier.minimumInteractiveComponentSize(), which is more dynamic and will respect user accessibility settings too
There was a problem hiding this comment.
Would this be compatible while using Jewel, since this is with Material?
There was a problem hiding this comment.
Good point. Maybe look at their docs and see if they have anything comparable?
There was a problem hiding this comment.
Could I use wrapContentSize() and set unbounded = true to account for accessibility?
According to the docs:
"Allow the content to measure at its desired size without regard for the incoming measurement minimum width or minimum height constraints, and, if unbounded is true, also without regard for the incoming maximum constraints. If the content's measured size is smaller than the minimum size constraint, align it within that minimum sized space. If the content's measured size is larger than the maximum size constraint (only possible when unbounded is true), align within the maximum space."
There was a problem hiding this comment.
not exactly. For example - a vector can be scaled to any size so it may not have an intrinsic size to be wrapped. I would try the jewel or intellij platform docs first, I assume there are some standard sizes for IJ iconography/buttons
Adding Circuit logic, which has a Chat Screen state, UI (ChatWindowUi object) and Presenter which handles the sending logic. I followed the tutorial and Project Gen for reference, but I also haven't used Circuit before, so I would appreciate any feedback.
f27ddc1