feat(avatar): add loading state to avatar and AIConversation #5777
Conversation
🦋 Changeset detectedLatest commit: 40a8945 The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| )} | ||
| > | ||
| <View className={ComponentClassName.AIConversationMessageAvatar}> | ||
| <Avatar isLoading>{avatar?.avatar}</Avatar> |
There was a problem hiding this comment.
If the intention of adding isLoading to the Avatar so it can render a loading indicator here seems like we could just provide the loading component as children here, e.g.:
<Avatar>{isLoading ? <LoadingComp /> : avatar?.avatar}</Avatar>
There was a problem hiding this comment.
We could, but I don't think we should. If you look at the gif the loading avatar shows the avatar icon and the circular loader so your could would need to include both a Loader component and the avatar for the loading state plus some CSS to get it positioned correctly. I think an isLoading prop for an Avatar primitive makes sense in general too.
There was a problem hiding this comment.
I do somewhat think the isLoading prop is very specific to our use case, but maybe that's not an issue since we're the primary user of this primitive right now
There was a problem hiding this comment.
Ultimately feel coupling loading state/rendering logic inside a UI primitive leads to long term complications with extension of the primitive. Adding isLoading to Avatar feels like a bit of a foot gun, since as a consumer i now need to make sure that I'm removing isLoading/setting to false if my avatar is loaded, in addition to the mental overhead of the existing behavior of the src and children props.
In this case providing a loading component would just require passing a className to our existing Loader component. IMO the Loader itself should just work out of the box for use cases where it is provided as a placeholder.
Not blocking, but do feel like we gain more from composition and lose flexibility by adding behavioral props
| )} | ||
| > | ||
| <View className={ComponentClassName.AIConversationMessageAvatar}> | ||
| <Avatar isLoading>{avatar?.avatar}</Avatar> |
There was a problem hiding this comment.
I do somewhat think the isLoading prop is very specific to our use case, but maybe that's not an issue since we're the primary user of this primitive right now
Description of changes
Added
isLoadingprop to Avatar primitive and added a loading state in the AIConversation which uses it.Issue #, if available
Description of how you validated changes
Checklist
yarn testpasses and tests are updated/addeddocs,e2e,examples, or other private packages.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.