Page MenuHomePhabricator

[web] Add fake pending thread during chat creation
ClosedPublic

Authored by jakub on Aug 24 2022, 1:27 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 20, 6:53 PM
Unknown Object (File)
Sat, Apr 20, 6:53 PM
Unknown Object (File)
Sat, Apr 20, 6:53 PM
Unknown Object (File)
Sat, Apr 20, 6:53 PM
Unknown Object (File)
Sat, Apr 20, 6:53 PM
Unknown Object (File)
Sat, Apr 20, 6:53 PM
Unknown Object (File)
Sat, Apr 20, 6:53 PM
Unknown Object (File)
Sat, Apr 20, 6:50 PM

Details

Summary

Context: here

Adding pending thread mock with "New thread" title, when we haven't chosen any users yet in chat creation mode.

Test Plan

I. Appearing:

During creating new chat:

  1. Go to web
  2. Click on Create new chat
  3. Mock with "New thread" title appear

Via url:

  1. Go to /chat/thread/new/ subpage (or refresh page, when you are already on it)
  2. Mock with "New thread" title appear

Via removing all selected users from thread composer:

  1. Go to chat creation mode
  2. Add any users
  3. Remove them
  4. Mock with "New thread" title appear

II. Disappearing:

Via selecting any users:

  1. Go to chat creation mode
  2. Select any users
  3. Active mock thread changes to pending or already existed thread

Via closing chat composer:

  1. Go to chat creation mode
  2. Close thread composer
  3. Mock with "New thread" tittle disappear

III. Additional:

  • Mock thread works exactly the same as normal pending thread in ChatThreadComposer, especially during selecting and removing users or switching in creation mode between pending and already existing threads.
  • Improved thread list behaviour (after closing ChatThreadComposer) is implemented in next diff (D4940)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jakub edited the test plan for this revision. (Show Details)
jakub added reviewers: tomek, jacek.
tomek requested changes to this revision.Aug 25 2022, 11:23 AM

Could you expand your test plan a bit by including scenarios like e.g. going from pending to realized thread, refreshing the page, adding and removing users from an input, etc.

web/chat/chat-message-list-container.react.js
57–65 ↗(On Diff #15907)

Is it really a good idea to use a ref here? I'm not saying it isn't but would be great if you could explain that.

This revision now requires changes to proceed.Aug 25 2022, 11:23 AM
web/chat/chat-message-list-container.react.js
57–65 ↗(On Diff #15907)

I wanted to avoid unnecessary re-rendering useMemo hook below. Holding this thread as plain object could trigger re-render of mentioned hook every component re-render, because eslint requires to add it to dependencies.
Alternatively, we could also use useMemo instead useRef, but I choose useRef because I wanted to keep the same convention as in pendingPrivateThread

web/chat/chat-message-list-container.react.js
57–65 ↗(On Diff #15907)

You should avoid calling createPendingThread again on every render, which is currently happening. useMemo is a good bet

web/chat/chat-message-list-container.react.js
57–65 ↗(On Diff #15907)

Ok, I will change that

tomek requested changes to this revision.Aug 29 2022, 3:18 AM
tomek added inline comments.
web/chat/chat-message-list-container.react.js
63 ↗(On Diff #16044)

Why do we need to override this?

64 ↗(On Diff #16044)

Can we use name parameter that createPendingThread accepts?

65 ↗(On Diff #16044)

What happens if we use the value that createPendingThread provides?

This revision now requires changes to proceed.Aug 29 2022, 3:18 AM
web/chat/chat-message-list-container.react.js
63 ↗(On Diff #16044)

To avoid matching with private thread, we need to distinguish private thread id and fake "new thread" id. Because in createPendingThread we couldn't pass by default threadID, we need to override this in this way.
Another solution is to add threadID as an optional argument in createPendingThread function.

64 ↗(On Diff #16044)

It's pretty weird, but when I tried it last time, it didn't work. But now I tested it and it works. So we can use it.

65 ↗(On Diff #16044)

I didn't check it before, but community parameter will also be null, so it could be removed

Removing unnecessary props and set name param as createPendingThread argument

tomek requested changes to this revision.Aug 29 2022, 9:26 AM

Looks good! Just one last question inline.

web/chat/chat-message-list-container.react.js
63 ↗(On Diff #16044)

Where does this matching happen? In const threadInfo = React.useMemo(() => { memo we're using early return

if (userInfoInputArray.length === 0) {
  return pendingNewThread;
}

Are there any other places where we would like to avoid the matching?

This revision now requires changes to proceed.Aug 29 2022, 9:26 AM
web/chat/chat-message-list-container.react.js
63 ↗(On Diff #16044)

App also match e.g. last message with existing thread using threadID. Now, when we create a new thread, there is "No message" communique below the thread name.
{F153203}
When we keep the value that createPendingThread provides by default, our app will match it with an existing private thread.
{F153205}
Also, I use it in next diff, to treat it as normal pending thread and to handle unique onClick event behaviour on it.

tomek added inline comments.
web/chat/chat-message-list-container.react.js
63 ↗(On Diff #16044)

These files do not show for me. I remember we had the solution described somewhere but not sure where.

This revision is now accepted and ready to land.Sep 2 2022, 4:09 AM