Page MenuHomePhabricator

[web] Introduce `ChatThreadComposer` component for selecting users to new thread
ClosedPublic

Authored by jacek on Jul 7 2022, 1:58 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 24, 12:46 AM
Unknown Object (File)
Tue, Nov 12, 1:11 AM
Unknown Object (File)
Tue, Nov 12, 1:10 AM
Unknown Object (File)
Tue, Nov 12, 1:10 AM
Unknown Object (File)
Tue, Nov 12, 1:10 AM
Unknown Object (File)
Tue, Nov 12, 1:10 AM
Unknown Object (File)
Tue, Nov 12, 1:10 AM
Unknown Object (File)
Tue, Nov 12, 1:10 AM

Details

Summary

Introducing main component used in thread creation view, that allows selecting users to new thread and displays list of found users:

Screenshot_Google Chrome_2022-07-07_105057.png (941×1 px, 73 KB)

The component doesn't contain callbacks implementation yet. The diff focuses mostly on UI. The callbacks will be added in following diffs.

Test Plan

The component is not used in UI yet. I tested it after introducing chat creation view in next diffs.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

web/chat/chat-thread-composer.react.js
42–44 ↗(On Diff #14241)

eslint-disable-next-line no-unused-vars will be removed in next diff with functions implementation

abosh added inline comments.
web/chat/chat-thread-composer.css
18 ↗(On Diff #14241)

Nit: the default value of flex-direction is row, so it doesn't need to be specified:
{F94860}

Read more on W3Schools.

(also applies to some other uses in this file)

web/chat/chat-thread-composer.react.js
50 ↗(On Diff #14241)

We only need to check if usernameInputText is falsy here. If it is, its length will be 0 (or it will be null or undefined). If it isn't, its length will be greater than 0.

This revision now requires changes to proceed.Jul 7 2022, 7:33 AM
web/chat/chat-thread-composer.css
18 ↗(On Diff #14241)

Agree - it doesn't need to be specified
For me, it makes code more readable to explicitly define direction instead of wondering what is the default one while reading the code.
We have multiple uses of flex-direction: row; in our codebase by different authors, and I think it should also be left here.

web/chat/chat-thread-composer.react.js
50 ↗(On Diff #14241)

You're absolutely right. Thanks for catching!

abosh added inline comments.
web/chat/chat-thread-composer.css
48 ↗(On Diff #14355)

Make sure to omit units for 0 values in CSS

In the original code, with four values specified, the paddings apply like this:

  • top: 0
  • right: 12px
  • bottom: 8px
  • left: 12px

In the edit, with three values specified, the paddings apply like this:

  • top: 0
  • left and right: 12px
  • bottom: 8px

So, the paddings are the same and this is a bit more concise. Yay!

Read more on the MDN Web Docs:
{F96225}

18 ↗(On Diff #14241)

Makes sense!

web/chat/chat-thread-composer.react.js
80 ↗(On Diff #14355)

I don't think you need the null here:
{F96221}

107 ↗(On Diff #14355)

on native, we use "username" as the placeholder text. Not sure if that's the best one, but do you think this should match?
{F96211}

However, the discrepancy between native and web is also a thing. For example, in a thread, native says "Send a message..." whereas web has "Type your message." So this isn't necessary, but worth thinking about.

This revision now requires changes to proceed.Jul 8 2022, 8:12 AM
web/chat/chat-thread-composer.css
48 ↗(On Diff #14355)

Thanks!

web/chat/chat-thread-composer.react.js
80 ↗(On Diff #14355)

Probably not, although I prefer returning explicitly null for no-component case, because it indicates that tagsList is by purpose an empty component - and not undefined one.

107 ↗(On Diff #14355)

It's hard to say, because we do not have design for this component in Figma. I have the feeling, that this component in native doesn't contain anywhere information that we are searching users for creating a new thread, so used the longer text here.
However, I'm not completely sure about it.
Maybe other team members will have suggestion what should be here (@palys-swm, @atul)

web/chat/chat-thread-composer.react.js
80 ↗(On Diff #14355)

OK, as long as you have a rationale, that makes sense!

Looks good to me, but would like @atul or @palys-swm's input as well since this diff creates an entirely new component.

This revision is now accepted and ready to land.Jul 13 2022, 1:05 PM

Resigning to put this on @atul and @palys-swm's queue.

This revision now requires review to proceed.Jul 14 2022, 12:29 PM
In D4461#129178, @yayabosh wrote:

Resigning to put this on @atul and @palys-swm's queue.

Yeah this is the sort of situation where being able to set "atul AND/OR palys-swm" as blocking reviewer would be handy.

Not sure what the protocol for this sort of thing is, but in the past I've added multiple people as blocking and then removed "blocking" status on the other reviewers as soon as one blocking reviewer accepts.

cc @ashoat

Yup, I agree. @ashoat suggested I resign in this case (since I had already stated it looks good to me, but I wanted one of you to take a look at it).

In D4461#129183, @yayabosh wrote:

Yup, I agree. @ashoat suggested I resign in this case (since I had already stated it looks good to me, but I wanted one of you to take a look at it).

Makes sense 👍🏽

tomek added a reviewer: ashoat.

Looks ok. Adding @ashoat so he can verify if the UI looks good

web/chat/chat-thread-composer.react.js
20

What's the meaning of this prop?

37

What do you think about putting this in a separate memo?

50

Could you explain a bit why we're returning null when !usernameInputText && userInfoInputArray.length?

web/chat/chat-thread-composer.react.js
37

Makes sense

50

The idea is to display user suggestions list when there is empty input only when no users have been selected.
It's how it works on native.
When we start creating a thread, we immediately receive list of users, which covers the screen. However, when we select some users to a chat, we display list of suggested users only if we input some text - so it doesn't cover chat all the time:

web/chat/chat-thread-composer.react.js
20

It's redundant and I'll remove it.
It was used to change styling (user list height) so the list doesn't cover the whole view of thread (that matches selected set of users)

UI looks good to me!! I think we can move forward with it as-is. Two quick notes I'll mention (no changes necessary, I just want to mention):

  1. The input mechanism is a little different than native. On native the tags appear in the TextInput directly, whereas here they appear below
  2. It's a bit strange how you can see a thread in the inbox when you are creating a new one, but when there is an existing one it doesn't show in the inbox. I think this is because the thread appears in the inbox but it's not near the top. We could maybe make a Backlog task to make this better (either "zoom" the scrollbar to the part of the inbox that has the thread, or move the thread to the top when it's selected)
This revision is now accepted and ready to land.Jul 19 2022, 8:51 AM

rebase & remove redundant prop & follow Tomeks suggestion with separate memo

We could maybe make a Backlog task to make this better (either "zoom" the scrollbar to the part of the inbox that has the thread, or move the thread to the top when it's selected)

Would be good to do this before landing

We could maybe make a Backlog task to make this better (either "zoom" the scrollbar to the part of the inbox that has the thread, or move the thread to the top when it's selected)

Would be good to do this before landing

Created follow-up diff with the fix: https://phab.comm.dev/D4615. It moves selected chat in creation mode to the top of the list.