[web] [fix] arrows touch per IRL chat with ashoat
ClosedPublic
Actions

Authored by • benschac on Feb 10 2022, 9:00 AM.

Details

Reviewers

atul
ashoat

Commits

rCOMM5ae973e85b43: [web] [fix] arrows touch per IRL chat with ashoat

Summary

had to toggle menu button size to make arrows touch. Additionally, there wasn't any design for sub thread unread menu on hover.

Image 2022-02-10 at 11.57.37 AM.jpg (724×934 px, 61 KB)

Test Plan

make sure it looks like the screen shot

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• benschac created this revision.Feb 10 2022, 9:00 AM

Herald added subscribers: • karol, • adrian, tomek, ashoat. · View Herald TranscriptFeb 10 2022, 9:00 AM

• benschac edited the summary of this revision. (Show Details)Feb 10 2022, 9:00 AM

Harbormaster completed remote builds in B6725: Diff 9541.Feb 10 2022, 9:05 AM

• benschac requested review of this revision.Feb 10 2022, 9:05 AM

• benschac added a reviewer: ashoat.Feb 10 2022, 5:58 PM

@benschac, if you ever feel the need to short-circuit the review process please communicate clearly the purpose for the urgency

web/chat/chat-thread-list-item-menu.react.js
26 ↗	(On Diff #9541)	Is this really the right solution? Generally this is a React anti-pattern... a child component shouldn't need to know the context in which it's being rendered. Can you come up with a way to avoid this problem? It's not even clear to me why we can't use the same design in all places that this component is rendered What's "submenu"? I think it needs a better name if you're going to do this If you really want to do something like this, you should probably do a string enum like `+renderStyle: 'submenu' \| 'somethingElse'`

This revision now requires changes to proceed.Feb 10 2022, 8:53 PM

address diff comments

web/chat/chat-thread-list-item-menu.react.js
26 ↗	(On Diff #9541)	I think so. We're using the same component in multiple contexts that call for different design. `submenu` --> thread. Updated it in my diff. did `chat` and `thread` --> does that make sense?

Harbormaster completed remote builds in B6753: Diff 9574.Feb 11 2022, 10:27 AM

We're using the same component in multiple contexts that call for different design.

Before landing, can you share screenshots of the two different designs in Figma, so it's clear to your reviewers why we need this new renderStyle prop?

This revision is now accepted and ready to land.Feb 13 2022, 8:54 PM

In D3165#84763, @ashoat wrote:

We're using the same component in multiple contexts that call for different design.

Before landing, can you share screenshots of the two different designs in Figma, so it's clear to your reviewers why we need this new renderStyle prop?

Sure:

the arrows on web figma don't have multiple threads. After chatting with ashoat, we made the decision to show multiple threads like this

After:

Image 2022-02-14 at 11.20.50 AM.jpg (596×982 px, 50 KB)

Why render style:

Additionally, there wasn't a unread button shown in figma either. In order to keep the arrows touching + unread buttions we needed a smaller menu buttion for threads.

Image 2022-02-14 at 11.22.42 AM.jpg (458×960 px, 42 KB)

Link to figma: https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=1170%3A77104

Closed by commit rCOMM5ae973e85b43: [web] [fix] arrows touch per IRL chat with ashoat (authored by • benschac). · Explain WhyFeb 14 2022, 8:25 AM

This revision was automatically updated to reflect the committed changes.

• benschac added a commit: rCOMM5ae973e85b43: [web] [fix] arrows touch per IRL chat with ashoat.

@benschac: After seeing your screenshots, and then re-reading the diff description, I think I finally understand what's going on now.

What I understand now: we agreed that we wanted the line to be continuous, but you didn't see a design in Figma so you had to make one up. You wanted to minimize the amount of work, and to do that you wanted to compose the line by only using the icon. The easiest way to do that was to hack the design and make the height of the thread unit shorter.

I really wish you had taken the time to explain this in more detail in the diff description. The screenshots that you later provided were helpful, and should've been there originally. Additionally, I wish your diff description was longer, and looked more like my last paragraph in terms of the level of detail.

Anyways: I don't agree with this design hack and I wish we had had an explicit discussion about it. Let's follow-up in a separate task. Creating ENG-766 to track.