Skip to content
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

[🐴] Remove extra spinner states from chat screen #3947

Merged
merged 5 commits into from
May 10, 2024

Conversation

haileyok
Copy link
Contributor

Cherry picking this out of #3945, because it's really separate from that PR and I'd like to get this one in to test on device.

Copy link

render bot commented May 10, 2024

Copy link

github-actions bot commented May 10, 2024

Old size New size Diff
6.9 MB 6.9 MB 554 B (0.01%)

@@ -553,7 +553,7 @@ export class Convo {
{
cursor: nextCursor,
convoId: this.convoId,
limit: isNative ? 40 : 60,
limit: isNative ? 30 : 60,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be the same as initialNumToRender in messagesList. If it's larger, there will be a scroll animation on render.

I'd like to keep this below 30 if we can, especially whenever we start considering adding other content types than text (honestly we might need to decrease the number when that happens, at least on native)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heard chef 🫡

Comment on lines -59 to -69
if (
convo.status === ConvoStatus.Uninitialized ||
convo.status === ConvoStatus.Initializing
) {
return (
<CenteredView style={a.flex_1} sideBorders>
<Header />
<ListMaybePlaceholder isLoading />
</CenteredView>
)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented on other PR already, but want to discuss this a bit. Not necessarily a blocker but want to be sure we can't do this another way.

Copy link
Member

@mozzius mozzius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

@haileyok haileyok merged commit 1a90426 into main May 10, 2024
6 checks passed
@haileyok haileyok deleted the hailey/remove-extra-spinners branch May 10, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants