Page MenuHomePhabricator
Differential D4939

[web] Add fake pending thread during chat creation
ClosedPublic
Actions

Authored by • jakub on Aug 24 2022, 1:27 AM.
Tags
None
Referenced Files
F5107771: D4939.id16552.diff
Tue, Apr 1, 11:59 AM
F5107770: D4939.id16550.diff
Tue, Apr 1, 11:59 AM
F5107769: D4939.id16497.diff
Tue, Apr 1, 11:59 AM
F5107768: D4939.id16061.diff
Tue, Apr 1, 11:59 AM
F5107767: D4939.id15907.diff
Tue, Apr 1, 11:59 AM
F5107766: D4939.id.diff
Tue, Apr 1, 11:59 AM
F5107765: D4939.diff
Tue, Apr 1, 11:59 AM
Unknown Object (File)
Mon, Mar 31, 5:19 PM
View All Files
Subscribers
• abosh
ashoat
atul
tomek

Details

Reviewers
tomek
• jacek
atul
Commits
rCOMM34623ec353e4: [web] Add fake pending thread during chat creation
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
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• jakub created this revision.Aug 24 2022, 1:27 AM
Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald TranscriptAug 24 2022, 1:27 AM
Harbormaster completed remote builds in B11569: Diff 15907.Aug 24 2022, 1:37 AM
• jakub requested review of this revision.Aug 24 2022, 1:37 AM
• jakub edited the summary of this revision. (Show Details)Aug 24 2022, 3:21 AM
• jakub edited the test plan for this revision. (Show Details)
• jakub added reviewers: tomek, • jacek.
• jakub added a child revision: D4940: [web] Change ThreadList behaviour after hiding ChatThreadComposer.Aug 24 2022, 4:19 AM
• jakub added a reviewer: atul.Aug 24 2022, 4:38 AM
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
• jakub edited the test plan for this revision. (Show Details)Aug 26 2022, 1:03 AM
• jakub added inline comments.Aug 26 2022, 5:34 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

ashoat added inline comments.Aug 26 2022, 11:51 AM
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

• jakub added inline comments.Aug 29 2022, 2:42 AM
web/chat/chat-message-list-container.react.js
57–65 ↗(On Diff #15907)

Ok, I will change that

• jakub updated this revision to Diff 16044.Aug 29 2022, 2:44 AM

Changing useRef to useMemo

Harbormaster completed remote builds in B11668: Diff 16044.Aug 29 2022, 2:54 AM
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
• jakub added inline comments.Aug 29 2022, 4:37 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

• jakub updated this revision to Diff 16061.Aug 29 2022, 6:57 AM

Removing unnecessary props and set name param as createPendingThread argument

Harbormaster completed remote builds in B11683: Diff 16061.Aug 29 2022, 7:07 AM
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
• jakub added inline comments.Aug 30 2022, 2:09 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.

• jakub requested review of this revision.Aug 31 2022, 6:37 AM
tomek accepted this revision.Sep 2 2022, 4:09 AM
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
• jakub updated this revision to Diff 16497.Sep 8 2022, 6:33 AM

Rebase

Harbormaster completed remote builds in B12015: Diff 16497.Sep 8 2022, 6:42 AM
• jakub updated this revision to Diff 16550.Sep 9 2022, 4:52 AM

Rebase before landing

Harbormaster completed remote builds in B12051: Diff 16550.Sep 9 2022, 5:02 AM
Closed by commit rCOMM34623ec353e4: [web] Add fake pending thread during chat creation (authored by • jakub). · Explain WhySep 9 2022, 5:03 AM
This revision was automatically updated to reflect the committed changes.
• jakub added a commit: rCOMM34623ec353e4: [web] Add fake pending thread during chat creation.

Revision Contents
Changeset List

PathSize
web/
chat/
19 lines

Diff 16550

View Options

web/chat/chat-message-list-container.react.js

// @flow // @flow
import { HeaderBackButton as BaseHeaderBackButton } from '@react-navigation/elements'; import { HeaderBackButton as BaseHeaderBackButton } from '@react-navigation/elements';
import invariant from 'invariant';
import * as React from 'react'; import * as React from 'react';
import { Text } from 'react-native'; import { Alert, Text } from 'react-native';
import { TouchableOpacity } from 'react-native-gesture-handler'; import { TouchableOpacity } from 'react-native-gesture-handler';
import type { NavigationRoute } from '../navigation/route-names';
import { useColors } from '../themes/colors.js'; import { useColors } from '../themes/colors.js';
type Props = React.ElementConfig<typeof BaseHeaderBackButton>; type ChangeRolesHeaderLeftButtonProps = {
function ChangeRolesHeaderLeftButton(props: Props): React.Node { +route: NavigationRoute<'ChangeRolesScreen'>,
...React.ElementConfig<typeof BaseHeaderBackButton>,
};
function ChangeRolesHeaderLeftButton(
props: ChangeRolesHeaderLeftButtonProps,
): React.Node {
const { memberInfo, role: selectedRole } = props.route.params;
const { role: memberRole } = memberInfo;
const onCancel = React.useCallback(() => {
const { onPress } = props;
invariant(onPress, 'onPress must be defined');
if (selectedRole === memberRole) {
onPress();
return;
}
Alert.alert(
'Unsaved Changes',
'You have unsaved changes, are you sure you want to cancel? You will lose all your progress',
[
{ text: 'Cancel', onPress },
{ text: 'Stay', style: 'cancel' },
],
{ cancelable: true },
);
}, [memberRole, props, selectedRole]);
rohanAuthorUnsubmitted
Done
Inline Actions

I think the wording here may be a bit confusing. In my experience, 'cancel' typically means close the alert, and there's another option like 'confirm' or something that will mean you're ok with discarding changes.

Here it seems like 'cancel' means we're ok with discarding changes. Not sure if it's just me though (cc @ted)

rohan: I think the wording here may be a bit confusing. In my experience, 'cancel' typically means…
ashoatUnsubmitted
Not Done
Inline Actions

What do we do in the current edit-message workflow? Heads-up I generally handle copy, not @ted

ashoat: What do we do in the current edit-message workflow? Heads-up I generally handle copy, not @ted
rohanAuthorUnsubmitted
Done
Inline Actions

Ah my bad. It looks like in message editing: Continue editing closes the alert and continues editing while Discard edit closes the alert and exits editing

rohan: Ah my bad. It looks like in message editing: `Continue editing` closes the alert and continues…
ashoatUnsubmitted
Not Done
Inline Actions
  1. I agree we shouldn't use the term Cancel here... how about Leave?
  2. I don't think we need { cancelable: true }... we can just default to Stay if the user dismisses the Alert
  3. Can we try to match copy for title / description with the edit-message workflow?
ashoat: 1. I agree we shouldn't use the term `Cancel` here... how about `Leave`? 2. I don't think we…
rohanAuthorUnsubmitted
Done
Inline Actions

Yeah that makes sense, I'll update the diff

rohan: Yeah that makes sense, I'll update the diff
const { panelForegroundSecondaryLabel } = useColors(); const { panelForegroundSecondaryLabel } = useColors();
const labelStyle = React.useMemo( const labelStyle = React.useMemo(
() => ({ () => ({
color: panelForegroundSecondaryLabel, color: panelForegroundSecondaryLabel,
marginLeft: 10, marginLeft: 10,
}), }),
[panelForegroundSecondaryLabel], [panelForegroundSecondaryLabel],
); );
return ( return (
<TouchableOpacity onPress={props.onPress}> <TouchableOpacity onPress={onCancel}>
<Text style={labelStyle}>Cancel</Text> <Text style={labelStyle}>Cancel</Text>
</TouchableOpacity> </TouchableOpacity>
); );
} }
export default ChangeRolesHeaderLeftButton; export default ChangeRolesHeaderLeftButton;