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

fix: chat not being scrolled to bottom (images) #4406

Closed
wants to merge 1 commit into from

Conversation

WofWca
Copy link
Collaborator

@WofWca WofWca commented Dec 12, 2024

Closes #4404.

The bug was introduced in db83279.
Prior to that all attachments had a fixed height of 200px.

This should also eliminate layout shifting when scrolling the chat,
and when new messages arrive.

An alternative solution that would still have layout shifting,
but at least keep the chat "anchored" to bottom would be
#3753 (comment),
but it's not supported well by browsers.

I tested it, and verified that images have proper dimensions now even before they get displayed, and that it wasn't the case prior to this MR.

@WofWca WofWca force-pushed the wofwca/fix-images-layout-shift branch from 5bb3710 to af0c8a4 Compare December 12, 2024 13:24
@WofWca WofWca marked this pull request as ready for review December 12, 2024 13:25
@WofWca WofWca requested a review from nicodh December 12, 2024 13:25
Closes #4404.

The bug was introduced in db83279.
Prior to that all attachments had a fixed height of 200px.

This should also eliminate layout shifting when scrolling the chat,
and when new messages arrive.

An alternative solution that would still have layout shifting,
but at least keep the chat "anchored" to bottom would be
#3753 (comment),
but it's not supported well by browsers.

Co-authored-by: Max Philippov <[email protected]>
@WofWca WofWca force-pushed the wofwca/fix-images-layout-shift branch from af0c8a4 to 1d2508f Compare December 12, 2024 13:31
@WofWca WofWca marked this pull request as draft December 12, 2024 13:37
@WofWca
Copy link
Collaborator Author

WofWca commented Dec 12, 2024

Wait, looks like this adds unnecessary padding for big images:

image

Copy link
Contributor

@nicodh nicodh left a comment

Choose a reason for hiding this comment

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

Tested and solves the problem!

return {
height:
message.dimensionsHeight > 0 && message.dimensionsHeight < 100_000
? message.dimensionsHeight
Copy link
Contributor

Choose a reason for hiding this comment

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

why we have a limit of 100_000 here? Would 10_000 not be enough?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably. But it's just a sanity check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe someone has 4 4K monitors stacked on top of each other on their wall 🤷‍♀️

@nicodh
Copy link
Contributor

nicodh commented Dec 12, 2024

Wait, looks like this adds unnecessary padding for big images:
Yes since the height stays the same when rescaling the whole window while the image keeps it ratio

Copy link
Contributor

@nicodh nicodh left a comment

Choose a reason for hiding this comment

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

It seems this needs more investigation... :-(

@WofWca
Copy link
Collaborator Author

WofWca commented Dec 12, 2024

OK, I'll close this and let's continue the discussion in the relevant issue.

@WofWca WofWca closed this Dec 12, 2024
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.

chat "scrolls up" right after switching to it, because images load and shift height
2 participants