Skip to content

Task/conditional chat auto scroll#29

Open
stnichol wants to merge 3 commits intomainfrom
task/conditional-chat-auto-scroll
Open

Task/conditional chat auto scroll#29
stnichol wants to merge 3 commits intomainfrom
task/conditional-chat-auto-scroll

Conversation

@stnichol
Copy link
Collaborator

@stnichol stnichol commented Dec 1, 2020

📃 Summary

There is an additional check for auto-scrolling when a new message arrives in chat to see if the user is looking at a previous message. The windows double-scrollbar should be fixed as well (not sure how it looks on mac).

🔍 What's the new behavior?

The chat box will now only auto-scroll to the bottom if the user is at the bottom of the chat box.

🏷 Task Link

https://www.notion.so/Task-update-chat-functionality-for-easier-UI-for-user-f67527b9accc42aabd8fa3eaa619ea1b

↩️ Depends On

Link any other PRs here.

🧪 QA

  • List the steps to test the changes here.

Send messages in the chat until it overflows then scroll up to a previous message to see if sending a message auto-scrolls.
Also, open chat and send messages until it overflows on Mac to see if the scrollbar looks alright.

🛑 Risk Analysis

  • Does it update dependencies?

return (
bounds.top >= 0 &&
// using a magic number of 130
bounds.bottom <= (window.innerHeight || document.documentElement.clientHeight) - 130
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT: could you put parenthesis around each conditional statement

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah no prob, like this?
(bounds.top >= 0) &&
(bounds.bottom <= (window.innerHeight || document.documentElement.clientHeight) - 130)

Copy link
Collaborator

Choose a reason for hiding this comment

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

yup!

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