-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Threads Beta — Design implementation review #21502
Comments
This is non-trivial as this is part of our ScrollPanel pagination mechanism - https://github.com/matrix-org/matrix-react-sdk/blob/develop/docs/scrolling.md#bacat-bottom-aligned-clipped-at-top-scrolling
|
This comment was marked as outdated.
This comment was marked as outdated.
This already appears to be 8px, can you confirm? |
@janogarcia @t3chguy @gsouquet can I add these visual glitches to your list pls? (develop, latest):
|
This comment was marked as outdated.
This comment was marked as outdated.
Regarding “From a thread” we can probably improve it, given that if that footer comes up then it may result in multiple messages with the same footer if the thread had quick subsequent replies and one of them matches the query. If the result is a thread reply then any possible shown context around it will be also so no point each and every event having its own "From a thread" |
@t3chguy Thanks for sharing the technical background on the current timeline scrolling implementation. 👍 That said, we shouldn’t expect all scrolling lists in the app to behave the same way, as those are dependent on the content. What makes complete sense for the room timeline may not be the most usable pattern for other contexts. I know this is from a completely different context, with its own constraints, but we’ve implemented it already for iOS/Android and need this to work consistently across platforms. So, what if we make some tradeoffs to make it work? → For example, let’s say we’re able to get the desired scrolling behavior TACAB (top-aligned, clipped-at-bottom), but we don’t try to prevent the list from jumping when new elements are dynamically added/removed? Would that make it any easier? Jumps in the thread list, while not an ideal UX, would be far less frequent than jumps in a timeline which gets constantly updated. |
Unfortunately maintaining two scrolling implementations will be very maintenance expensive, preventing scroll jumps in infinite-scrolling lists is non-trivial.
Yes it would but the maintenance burden would still be large, it'd cause a lot of code duplication and lead to a lot of potential for bugs due to the low test coverage we have and are able to have over complex interactions like infinite scrollers.
A TACAB approach would have guaranteed scroll jumps every time you're back-paginating (scrolling up) due to the spinner rendering in & out. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
@t3chguy In that case, we can wait for the spec to be updated. That said, the screenshot you shared with example search results is showing a thread summary on the first message. Has this been implemented somehow for search result context messages (but not for the actual search result matching messages)? (iOS/Android are also showing thread summaries for root messages in search results. That’s why I assumed it was missing on Web. Unsure how they circumvented the current backend limitations and how performant it is.) |
As per my comment
In other words, its done on a best effort basis without having to hammer the server by asking for each event if it is a thread root. (i.e if user scrolled to it or it was recent or user opened the Thread Panel to cause all threads to get loaded in) Edit: we could more eagerly fetch all threads (as soon as you open the room, or when you perform a search) but both can cause the server to get hammered more than necessary and cause a lot more memory & bandwidth to get consumed. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Hmm no, it's not intentional... Both |
This comment was marked as resolved.
This comment was marked as resolved.
@janogarcia can you elaborate on this? It is the only non-blocked point which is not tracked in another issue |
@t3chguy Sure, we need to make sure we're applying the same vertical alignment properties/behavior for thread summaries in the thread list as those we defined a while ago for thread summaries in the main timeline. Ideally, we wouldn't need to define/update these styles in two separate places.
|
@janogarcia can you please confirm whether we have done enough here to close the ticket? |
So, the only issue pending to be addressed is the infinite scrolling behavior. More specifically, the limitation of the current implementation (BACAT), that prevents us from ordering the scrollable events from top to bottom. I’ve been thinking more about this lately and would like to share some updates:
From a UX perspective, we can’t presume that BACAT scrolling will work and serve best every possible scenario and interaction need (e.g., we already use the top-to-bottom scrolling pattern in the room list on the left sidebar and the people list on the right sidebar, although not for infinite items). We consider top-to-bottom scrolling a core interaction pattern worth the effort of solving. It surely has a considerable implementation cost, but it also has a similar or higher product/UX cost not having it. Now, given that we want to reuse this behavior in different contexts (mentions panel, files panel), meaning it’s not a Threads-specific limitation but a core interaction issue affecting our product in general, I think we could move this requirement out of the scope of the Exit Beta milestone for Threads and handle it separately. So, to recap:
What do you think? /cc @nadonomy CCing you as per a previous internal discussion. |
@novocaine @t3chguy Feel free to close this issue if you agree with the stuff above. |
The ordering isn't an issue to flip, the issue is the potential for a gap at the top due to how BACAT reserves space to prevent scroll jumps e.g by the spinner coming up and going away. TACAB scrolling has guaranteed scroll-jumps for lazily/infinite loaded lists as the spinner renders and then vanishes.
Great, though would be good to clarify whether by
You mean just flipping the order or TACAB which would trade scroll jumps for having no gap Thanks |
We want to be able to flip the order, top to bottom, with the first item being aligned to the top of the scrolling container, without any extraneous/additional white space on the top, as in the mockup below (a filterable/sortable list of items). I'd also expect no scroll position jumps when interacting with the scrolling behavior. If the spinner is introducing the problem, then we could think of a slightly different design for it. |
If we can replace the spinner with something that consumes no vertical space (to have to pop-in and out) then that would help |
We can probably hack BACAT to remove the paging behaviour for the threads list, as long as we can turn the spinner/no-spinner into a constant height element. Like the old youtube top-bar horizontal animation which I cannot find a reference to atm |
What if we did something like this? Let's say |
Can't recall that one, but would to see an example. |
Yup an overlaid loading state would work too, anything that doesn't cause the vertical bounce |
Alternatively, we could just show an animated loading bar on the bottom that would only take up 2 additional pixels. We could just keep that 2px gap on the bottom even when the loader is inactive. We could live with that, even if not ideal. We could also combine both for better feedback on what's being loaded, but it's probably getting a bit noisy. |
https://www.cssscript.com/demo/youtube-top-loader/ the loading bar was what I had in mind, combining both is also possible |
Thanks for the example! Yeah, basically the same stuff I suggested in the previous example. 👍 |
I just noticed Github has such a minimal loading animation also :D |
believe this is completed and can therefore be closed... |
@daniellekirkwood Related internal discussion on BACAT scrolling and sorting. That said, we probably don't plan to act on the scrolling implementation soon, so we may want to consider that as a standalone enhancement (affecting the threads list, but also the mentions and files panels) that we can separate from the Threads project, as suggested previously. |
Created an issue to keep track of the scrolling things here Will now close this issue. |
Resources
Thread list
.mx_EventTile_avatar
render a 36px image (instead of 24px)❌ Apr. 1 Now it's measuring 32px instead of 36px.
.mx_EventTile_avatar
top position to0
(instead oftop: -4px
)✋ Blocked by BACAT scrolling
❌ May. 26 It's been suggested to treat this enhancement separately from the Threads project. Threads Beta — Design implementation review #21502 (comment)
❌ Apr. 1 Now it's measuring 46px instead of 40px.
→ Figma
🐛 Apr. 1 Barely visible, the dot is almost completely cut off (macOS Chrome).
#61708b
)Thread list: Hover state
Fix split background colors. (screenshot)Tracked by Implement improved spacing for the thread list and timeline. #21759
Extend background color on all sides to add expected padding.Tracked by Implement improved spacing for the thread list and timeline. #21759
Thread list: Empty state
Refer to mockups.
Tip: Use “Reply in thread” when hovering over a message.
.→ Figma
🐛 Apr. 1 When visiting a room it suddenly changes to the previous version (the one without the tip) when it attempts to load the thread list for the first time.
Show all threads
font size to 15px.→ Figma
→ Figma
❌ Apr. 1 The gap is now bigger, get it 2px closer.
Thread view
❌ Apr. 1 It was 48px, now 52px, still not 60px.
❌ Apr. 1 Set
padding-top: 14px
for.mx_ThreadView .mx_EventTile
.Align timeline to the top of the timeline container.→ Figma
✋ Blocked by BACAT scrolling
❌ May. 26 We're dropping this requirement for the thread timeline.
View in room
action is missing in the root message contextual actions.Thread search: Keyword match in thread reply
margin-left: 8px;
for.mx_ThreadSummaryIcon::before
to bemargin-right: 8px;
), reduce top margin (makemargin-top: 8px
formx_ThreadSummaryIcon
).→ Figma
❌ Apr. 1 "From a thread" text should use secondary color. Also,
.mx_ThreadSummaryIcon
is now an inline element, and thus the 8px vertical margin is having no effect.mx_ThreadSummaryIcon
clickable, behaving the same way as its relatedmx_EventTile_line
.Thread search: Keyword match in thread root message
→ Figma
Tracked by Thread summary is not displayed for root messages in search results #21450
Refer to the annotations on how the flow should work for “Keyword match in root message”:
The text was updated successfully, but these errors were encountered: