Page MenuHomePhabricator

[web] [fix] give chat thread list breadCrumbs height
ClosedPublic

Authored by benschac on Mar 2 2022, 1:09 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 14, 4:24 AM
Unknown Object (File)
Tue, Nov 5, 9:56 PM
Unknown Object (File)
Tue, Nov 5, 12:51 AM
Unknown Object (File)
Oct 11 2024, 2:29 PM
Unknown Object (File)
Oct 11 2024, 2:09 PM
Unknown Object (File)
Oct 7 2024, 6:12 AM
Unknown Object (File)
Oct 7 2024, 6:12 AM
Unknown Object (File)
Oct 7 2024, 6:12 AM

Details

Summary

chat thread arrow layout is broken without breadcrumbs. If there isn't a parent thread, will just default to the thread name to not break the UI.

ios:

Image 2022-03-08 at 11.38.18 AM.jpg (1×1 px, 706 KB)

web:
Image 2022-03-08 at 11.40.49 AM.jpg (2×2 px, 942 KB)

https://linear.app/comm/issue/ENG-809/thread-arrows-are-not-aligned-correctly

Test Plan

arrow shouldn't loose it position if there isn't a parnet element. You can patch the code and toggle the conditional to see that the arrows layout never breaks.

Diff Detail

Repository
rCOMM Comm
Branch
fix-icon-paddings
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

this isn't ready.

benschac edited the test plan for this revision. (Show Details)

this isn't ready.

Is it ready now? Saw you updated it a minute later but just wanted to make sure

In D3328#89709, @atul wrote:

this isn't ready.

Is it ready now? Saw you updated it a minute later but just wanted to make sure

yes! I updated the diff to include something (the ui name) in the top line if it's the root conversation. I thought the conversation item would look off it was just empty space.

atul requested changes to this revision.Mar 3 2022, 10:52 AM

Hmm, I'm not sure the logic here is right?

wrt the image you attached as being fixed:

Image_2022-03-02_at_4.38.11_PM.jpg (410×870 px, 32 KB)

I'm pretty sure we should have GENESIS above "ashoat"

This revision now requires changes to proceed.Mar 3 2022, 10:52 AM
In D3328#89732, @atul wrote:

Hmm, I'm not sure the logic here is right?

wrt the image you attached as being fixed:

Image_2022-03-02_at_4.38.11_PM.jpg (410×870 px, 32 KB)

I'm pretty sure we should have GENESIS above "ashoat"

Sorry I should have added this to my description. Just updated it now:

The placeholder in the screenshot isn't the logic of the code. I didn't have a conversation in my local dev environment without a parent and just wanted to show what the placeholder would look like in practice.

I didn't have a conversation in my local dev environment without a parent

I think the GENESIS thread?

In D3328#89770, @atul wrote:

I didn't have a conversation in my local dev environment without a parent

I think the GENESIS thread?

Yeah, it's weird. On development, the genesis thread will show on the top line of the chat thread item. But, not in production. (screenshot below). Assuming that something changed on the backend and genesis thread is now returned as a default.

Development:

Image 2022-03-04 at 10.08.30 AM.jpg (2×2 px, 616 KB)

Production:
Image 2022-03-04 at 10.09.02 AM.jpg (2×1 px, 731 KB)

I'll close this diff, for now, and update the linear task.

I'll close this diff, for now, and update the linear task.

Taking this back. This issue still exists I updated the linear issue.

This revision now requires review to proceed.Mar 4 2022, 2:35 PM
benschac edited the summary of this revision. (Show Details)
benschac edited the summary of this revision. (Show Details)
ashoat requested changes to this revision.Mar 7 2022, 10:16 PM

Why are we hacking this instead of fixing useAncestorThreads so native behaves in a consistent way here?

This revision now requires changes to proceed.Mar 7 2022, 10:16 PM

@ashoat -- updated this diff and updated the useAncestorThreads hook so that the change is reflected in both native and web.

ashoat added a reviewer: atul.
ashoat added a subscriber: atul.

Seems fine to me, but wondering if we should just return ancestorThreads directly. Curious to get @atul's perspective

lib/shared/ancestor-threads.js
17–22

@atul did we ever conclude that we want to show the whole list here instead of excluding the current thread? I remember we had some discussion about this

@atul did we ever conclude that we want to show the whole list here instead of excluding the current thread? I remember we had some discussion about this

Yeah, at the time we decided to exclude the current thread from the path.

If I remember correctly:

  • We concluded that including the thread name in the "path" was redundant and didn't provide any new information. We figured that "the point" of the path was to help users figure out "where" a given thread was, and including the current thread didn't provide any value.
  • Visually, we didn't want the ancestor path to get overwhelmingly long at which point we "naively" cut off the end with ellipsis. We decided to re-assess this if we happened to do the "notion-style" interactive ellipsis in the thread list or if we decided to incorporate some sort of horizontal scrolling in the thread list (which we were both skeptical of).
  • But basically decided to weigh minimizing visual clutter over marginal (if any) benefit to user.

However, we did decide that we wanted to include the entire path in the ThreadSettings component (where we have the interactive horizontal scrolling ancestors component) because:

  • There was space for it
  • It made sense visually since we were using arrows to show relationships between threads and it would be weird to exclude the one the user was in
  • Clicking on the current thread in the ThreadSettings ancestry component navigates you to the thread, which keeps the navigation behavior symmetrical/predictable compared to the other threads listed
This revision is now accepted and ready to land.Mar 8 2022, 12:10 PM

Thanks for explaining that context in detail, @atul!