Details

Reviewers

tomek
atul
ashoat

Commits

rCOMM3fd1233147c1: [web] Change ThreadList behaviour after hiding ChatThreadComposer

Summary

Depends on D4939
Context: here

Changing behaviour after closing ChatThreadComposer.

Now, when we close thread composer on pending thread by clicking the X button, app will focus on the first thread on the top of the list

In other cases, app will focus on the currently generated or matched thread.

Also, this diff improves behavior of ChatThreadListItem onClick event. Now, clicking on currently active thread in chat creation mode doesn't close ChatThreadComposer.

Test Plan

Steps to test new ChatThreadComposer behavior:

Click Create new chat
Create pending thread (don't select any users or select set of users, which doesn't match to any existing thread)
Close composer by click X button

Now, app is currently focused on the first thread from the thread list.

In other cases, when we send a message to a new thread or we close ChatThreadComposer on an already existing thread, app keeps focus on the current thread.

Steps to test new ChatThreadListItem behavior:

Click Create new chat: a) Click on currently active ChatThreadListItem -> chat creation mode still active b) Click on any other ChatThreadListItem -> app switches active chat item to the new one and closes chat creation mode

Diff Detail

Repository

rCOMM Comm

Branch

jakub/temporary

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald TranscriptAug 24 2022, 2:05 AM

Harbormaster completed remote builds in B11570: Diff 15908.Aug 24 2022, 2:15 AM

• jakub requested review of this revision.Aug 24 2022, 2:15 AM

• jakub edited the summary of this revision. (Show Details)Aug 24 2022, 4:19 AM

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

• jakub added reviewers: tomek, • jacek.

• jakub added a parent revision: D4939: [web] Add fake pending thread during chat creation.

• jakub added a reviewer: atul.Aug 24 2022, 4:38 AM

ashoat added inline comments.Aug 24 2022, 9:10 AM

web/chat/chat-thread-composer.react.js
110 ↗	(On Diff #15908)	Let's use an enum, it's unclear what this boolean does at the callsite

• jakub added inline comments.Aug 25 2022, 2:24 AM

web/chat/chat-thread-composer.react.js
110 ↗	(On Diff #15908)	Sure, I will change that

Change boolean to enum and fix bug related to onClick on fake pending "New
thread"

Harbormaster completed remote builds in B11604: Diff 15962.Aug 25 2022, 3:09 AM

• jakub mentioned this in D4939: [web] Add fake pending thread during chat creation.Aug 26 2022, 1:03 AM

ashoat added inline comments.Aug 26 2022, 11:00 AM

web/chat/chat-thread-composer.react.js
26 ↗	(On Diff #15962)	Let's make this more specific. It should be clear at the callsite what the parameter does. Right now, it's not clear at the callsite what is being "kept" or "cleared"

• jakub added inline comments.Aug 28 2022, 9:26 PM

web/chat/chat-thread-composer.react.js
26 ↗	(On Diff #15962)	Could it be "keep-thread-id" and "clear-thread-id"/"change-thread-id"?

Updating diff after resolving conflicts

Harbormaster completed remote builds in B11684: Diff 16062.Aug 29 2022, 7:09 AM

tomek requested changes to this revision.Aug 30 2022, 9:13 AM

tomek added inline comments.

web/chat/chat-thread-composer.react.js
169–171 ↗	(On Diff #16062)	We should memoize the callback
26 ↗	(On Diff #15962)	Instead of focusing on the implementation detail that we use thread id, the name should focus on the difference in behavior. So it this case it should tell whether we're going to stay on the page or open something else.

This revision now requires changes to proceed.Aug 30 2022, 9:13 AM

• jakub added inline comments.Aug 31 2022, 2:53 AM

web/chat/chat-thread-composer.react.js
169–171 ↗	(On Diff #16062)	Sure

Memoize onClick callback and change enum value names

Harbormaster completed remote builds in B11733: Diff 16123.Aug 31 2022, 3:12 AM

I tested it a bit and discovered a couple of strange behaviors:

When we have a pending thread opened and we click it on threads list it gets closed - should we really do that?
When we have a pending thread opened with some users selected, clicking it doesn't result in it being closed - it remains displayed on the list and its message view is still presented but not in the search mode.

web/selectors/nav-selectors.js
145–170 ↗	(On Diff #16123)	We have duplicated logic here. Can we refactor it so that there's just one `dispatch`?

This revision now requires changes to proceed.Aug 31 2022, 5:14 AM

Change thread behaviour and refactor logic in nav-selectors.js

I changed both behaviours. Now, after clicking on active thread in create mode, app keeps focused on it and ThreadComposer doesn't close.
I also considered using a css class with pointer-event: none property instead to dynamically disable onClick event, but this solution has an impact on children pointer-events, so it could be problematic (e.g. onClick on ChatThreadListItemMenu)

web/selectors/nav-selectors.js
145–170 ↗	(On Diff #16123)	Sure

Harbormaster completed remote builds in B11803: Diff 16206.Sep 1 2022, 7:48 AM

I tested it and wasn't able to break it, so it works correctly. Added one question because I would like to understand it better why it works correctly.

web/chat/chat-thread-composer.react.js
121 ↗	(On Diff #16206)	Could you explain why setting empty string here works correctly?

This revision now requires changes to proceed.Sep 2 2022, 6:57 AM

• jakub added inline comments.Sep 8 2022, 4:18 AM

web/chat/chat-thread-composer.react.js
121 ↗	(On Diff #16206)	When we update redux state, every time new state is validated by `validateState` function. If we pass empty string or invalid threadID, this function will catch it and guarantee returning a most recent readed thread id. Here is part of code responsible for this.

Rebase

Harbormaster completed remote builds in B12016: Diff 16498.Sep 8 2022, 6:45 AM

Really nice! Just a couple minor comments inline.

web/chat/chat-thread-composer.react.js
121 ↗	(On Diff #16206)	Ok, that makes a lot of sense. We could consider determining this thread here, but it's better to let reducer figure it out. It would be a lot safer to use `null` instead of empty string, though.
web/chat/chat-thread-list-item.react.js
140 ↗	(On Diff #16498)	Can we create a callback that calls `onClick` conditionally and could be used here? From performance point of view it shouldn't matter, but it would be more readable to have this logic in a callback instead of setting the prop conditionally here.
web/selectors/nav-selectors.js
131 ↗	(On Diff #16498)	In the most recent solution we use `newThreadID` only in `chat-message-list-container` so maybe we should revert moving it and keep it in `chat-message-list-container`.

This revision now requires changes to proceed.Sep 8 2022, 6:49 AM

Change empty string to null, return newThreadID to the previous position and add
conditional callback for onClick

Harbormaster completed remote builds in B12020: Diff 16503.Sep 8 2022, 8:26 AM

atul removed a reviewer: • jacek.Sep 8 2022, 12:27 PM

tomek requested changes to this revision.Sep 9 2022, 4:11 AM

tomek added inline comments.

web/chat/chat-thread-list-item.react.js
50–53 ↗	(On Diff #16503)	Can we use a `useCallback` here? Using memo when we have a function might be confusing. On the other hand, memo is a little more performant in this case... but still, I would prefer readability here.

This revision now requires changes to proceed.Sep 9 2022, 4:11 AM

Changing useMemo to useCallback

Adding @ashoat because he had some comments

web/chat/chat-thread-list-item.react.js
52 ↗	(On Diff #16540)	I think it might a little more readable if we used an alternative instead

Harbormaster completed remote builds in B12041: Diff 16540.Sep 9 2022, 4:48 AM

• jakub mentioned this in rCOMM34623ec353e4: [web] Add fake pending thread during chat creation.Sep 9 2022, 5:03 AM

Change if condition to alternative form

Harbormaster completed remote builds in B12053: Diff 16553.Sep 9 2022, 5:18 AM

Mostly feedback about naming

web/chat/chat-thread-composer.react.js
26	Nit: can we use American English instead of British English? "Behavior" instead of "behaviour"
26	"Renew" is confusing here... let's say `reset-active-thread` instead It appears that the reset will not occur if the active thread is not pending. How about `reset-active-thread-if-pending`?
web/chat/chat-thread-list-item.react.js
44–57	This too should've been a separate diff. It took a second for me to realize why it was necessary... it would've been helpful to have a separate diff name / diff description to explain why this change is necessary At this point you can feel free to leave it in
50	`conditionalClick` is not a very helpful name. How about `selectItemIfNotActiveCreation`
web/selectors/nav-selectors.js
144–158	These changes appear to have nothing to do with this diff. They should've been a separate diff, but at this point I won't object to leaving them here

This revision now requires changes to proceed.Sep 15 2022, 12:29 PM

• jakub edited the summary of this revision. (Show Details)Sep 16 2022, 12:11 AM

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

Rebase branch and rename ActiveThreadBehaviour and conditionalOnClick

Looks great!!

This revision is now accepted and ready to land.Sep 16 2022, 6:30 AM

Rebase before landing

Harbormaster completed remote builds in B12246: Diff 16806.Sep 19 2022, 1:22 AM

Closed by commit rCOMM3fd1233147c1: [web] Change ThreadList behaviour after hiding ChatThreadComposer (authored by • jakub). · Explain WhySep 19 2022, 1:25 AM

This revision was automatically updated to reflect the committed changes.

• jakub added a commit: rCOMM3fd1233147c1: [web] Change ThreadList behaviour after hiding ChatThreadComposer.

[web] Change ThreadList behaviour after hiding ChatThreadComposer
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 16553

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

web/chat/chat-thread-list-item.react.js

web/selectors/nav-selectors.js

[web] Change ThreadList behaviour after hiding ChatThreadComposerClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 16553

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

web/chat/chat-thread-list-item.react.js

web/selectors/nav-selectors.js

[web] Change ThreadList behaviour after hiding ChatThreadComposer
ClosedPublic
Actions

Revision Contents
Changeset List