Add scrolling case for longer user message#2995
Conversation
| private getAnchorScrollTarget(message: ChatMessage): number { | ||
| const container = this.conversation.messagesContainer; | ||
| const containerRect = container.getBoundingClientRect(); | ||
| const messageRect = message.getBoundingClientRect(); | ||
| const messageTopInContainer = messageRect.top - containerRect.top + container.scrollTop; | ||
| const { clientHeight } = container; | ||
| const maxMessageCoverage = clientHeight * anchoredMessageMaxViewportFraction; | ||
| if (messageRect.height > maxMessageCoverage) { | ||
| return messageTopInContainer + messageRect.height - maxMessageCoverage; | ||
| } | ||
| return messageTopInContainer; | ||
| } | ||
|
|
There was a problem hiding this comment.
A couple of concerns with this PR:
- If you notice from the previous refactor, a significant effort was put in to avoid manual calculations like this to measure the dom and instead rely on native CSS browser features. Please take some time to brainstorm and consider how this can be done in CSS with the browser native layout system instead of manually calculating layout. Things like this should be a last resort with comments describing approaches considered.
- This PR adds no tests. Notice the previous refactor includes tests to cover behavior. Please review and add tests
- This change is not covered by storybook matrix tests. Make sure this case is covered in storybook matrix snapshot testing to prevent regressions.
There was a problem hiding this comment.
I did some digging and the only CSS solution I found was a sentinel element at the message bottom with scroll-margin-top: 20cqh, but it has the problem that short messages end up with empty space above them instead of being flush with the viewport top (because scroll-margin can't reference the element's own height). So I opted to add a comment explaining the need for these manual calculations.
I also added unit tests, but had some trouble with the storybook matrix tests as they render the component and take a screenshot immediately. The scroll behavior only triggers after a user sends a message, so the matrix story only shows the un-scrolled state. I could try to look for some workarounds for this such as waiting long enough for the scrolling animation to complete. However, I first wanted to ask if you think this is something worth pursuing.
There was a problem hiding this comment.
I'm OOO through 7/20, I can either take a look after I'm back in office or if need to look sooner @mollykreis may be able to review the approach and suggest alternatives / give the approval to merge.
In terms of alternatives to what is being proposed (concerns of lots of manual sizing calculations and concerns about timing assumptions / responsiveness to dynamic content) I'd have to look more closely when in office.
In terms of concerns with just how the implementation is done as-is, a concern I have is having dynamic style attributes being mutated directly on elements. Instead set custom property values that can be consumed via css (table examples). Want the definition of CSS styles to be static and can do things like min-height: calc(100% + var(--ni-private-conversation-shortfall-height));;
There was a problem hiding this comment.
Back in office but was asked to prioritize other tasks through this week so the earliest I can take a look myself is next week. Can work with @jattasNI to review the current changes to the chat conversation components.
There was a problem hiding this comment.
I've finally cleared most of my backlog from being OOO and should have bandwidth to review this PR next week.
Before I do, @Alexia-Claudia-Micu could you clarify some questions?
- Could you update the PR description to explain the current behavior and the desired behavior? The PR description links to a couple issues but those aren't enough to tell what this PR is doing.
- From my storybook experimentation, it looks like the current behavior is that new user messages get scrolled so that their top aligns with the top of the conversation. For tall messages this means the top is visible but the bottom isn't. The new behavior is that, for messages greater than 2 lines, the top is scrolled offscreen, the bottom 2.5 lines are visible, and space is available for the response. Is this confirmed as the desired behavior from UX or is it currently a dev proposal?
I haven't looked into the implementation yet but my gut feel is that it might take some brainstorming to meet Milan's architectural concerns while achieving the desired UX behavior. So I want to make sure it's worth investing in that before we do. An alternative would be to keep the behavior as-is for now and wait for user feedback. I learned today that the desktop teams plan to uptake this soon, so maybe their feedback will help us decide what behavior we want.
|
Talked to @Alexia-Claudia-Micu and they'll be OOO for a few weeks and can address feedback when back. Moving to draft for now. |
3 participants
Pull Request
🤨 Rationale
https://dev.azure.com/ni/DevCentral/_workitems/edit/3897966
https://dev.azure.com/ni/DevCentral/_workitems/edit/3943833
👩💻 Implementation
Added
anchoredMessageMaxViewportFraction= 0.2 to keep the user message within that height in the viewport.🧪 Testing
Example app testing.
✅ Checklist