Page MenuHomePhabricator
Differential D4912

[web] Refactor `MessageTooltip` component
ClosedPublic
Actions

Authored by • jacek on Aug 23 2022, 4:52 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Apr 14, 7:37 AM
Unknown Object (File)
Wed, Apr 9, 12:50 AM
Unknown Object (File)
Wed, Apr 9, 12:11 AM
Unknown Object (File)
Tue, Apr 8, 11:26 PM
Unknown Object (File)
Tue, Apr 8, 10:31 PM
Unknown Object (File)
Tue, Apr 8, 10:31 PM
Unknown Object (File)
Tue, Apr 8, 10:31 PM
Unknown Object (File)
Tue, Apr 8, 10:30 PM
View All Files
Subscribers
ashoat

Details

Reviewers
tomek
atul
• abosh
Commits
rCOMM36b1f6c71dcb: [web] Refactor `MessageTooltip` component
Summary

Major refactor of MessageTooltip that simplifies the component leaving only necessary properties and removing positioning logic out of the component

The new component:

Screenshot_Google Chrome_2022-08-23_143424.png (140×200 px, 10 KB)

Test Plan

Flow; Tested with the rest of diffs stack.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

• jacek created this revision.Aug 23 2022, 4:52 AM
• jacek held this revision as a draft.
Herald added a subscriber: ashoat. · View Herald TranscriptAug 23 2022, 4:52 AM
Harbormaster completed remote builds in B11530: Diff 15853.Aug 23 2022, 5:01 AM
• jacek added a parent revision: D4911: [web] Introduce tooltip container with positioning in `TooltipContext`.Aug 23 2022, 5:26 AM
• jacek added a child revision: D4913: [web] Modify `findTooltipPosition` to support new tooltip positioning logic.
• jacek edited the summary of this revision. (Show Details)Aug 23 2022, 5:34 AM
• jacek added inline comments.Aug 23 2022, 5:41 AM
web/chat/composed-message.react.js
132 ↗(On Diff #15853)

Will be changed in next diffs - change to avoid flow issue

web/chat/robotext-message.react.js
27 ↗(On Diff #15853)

Will be removed in next diffs

69 ↗(On Diff #15853)

Will be changed in next diffs

• jacek published this revision for review.Aug 23 2022, 8:06 AM
tomek accepted this revision.Aug 25 2022, 11:02 AM
tomek added inline comments.
web/chat/message-tooltip.css
6 ↗(On Diff #15853)

Please use variables in any new code

web/chat/message-tooltip.react.js
16–19 ↗(On Diff #15853)

If the classes are always enabled you can use a lot simpler syntax

This revision is now accepted and ready to land.Aug 25 2022, 11:02 AM
• abosh accepted this revision.Aug 25 2022, 11:25 AM
• abosh added inline comments.
web/chat/message-tooltip.react.js
11 ↗(On Diff #15853)

What's a case when messageTimestamp would be optional? Doesn't every message have a timestamp?

• jacek added inline comments.Aug 29 2022, 3:32 AM
web/chat/message-tooltip.react.js
11 ↗(On Diff #15853)

Yes, it doesn't have to be optional

• jacek updated this revision to Diff 16054.Aug 29 2022, 5:34 AM

Reabase & address feedback

Harbormaster completed remote builds in B11678: Diff 16054.Aug 29 2022, 5:44 AM
• jacek updated this revision to Diff 16072.Aug 29 2022, 7:20 AM

Used JS constants in styles to support correct measuring size

Harbormaster failed remote builds in B11686: Diff 16072!Aug 29 2022, 7:21 AM
Harbormaster completed remote builds in B11686: Diff 16072.Aug 29 2022, 7:54 AM
• jacek updated this revision to Diff 16189.Sep 1 2022, 6:55 AM

add alignment parameter

Harbormaster completed remote builds in B11786: Diff 16189.Sep 1 2022, 7:06 AM
• jacek updated this revision to Diff 16204.Sep 1 2022, 7:27 AM

fix css name

Harbormaster completed remote builds in B11801: Diff 16204.Sep 1 2022, 7:34 AM
Closed by commit rCOMM36b1f6c71dcb: [web] Refactor `MessageTooltip` component (authored by • jacek). · Explain WhySep 5 2022, 3:13 AM
This revision was automatically updated to reflect the committed changes.
• jacek added a commit: rCOMM36b1f6c71dcb: [web] Refactor `MessageTooltip` component.

Revision Contents
Changeset List

Diff 16288

View Options

web/chat/chat-constants.js

  • This file was added.
// @flow
import invariant from 'invariant';
import Alert from 'react-native/Libraries/Alert/Alert';
import {
sendReactionMessage,
sendReactionMessageActionTypes,
} from 'lib/actions/message-actions';
import type { BindServerCall, DispatchFunctions } from 'lib/utils/action-utils';
import type { TooltipRoute } from '../navigation/tooltip.react';
function onPressReact(
route:
| TooltipRoute<'TextMessageTooltipModal'>
| TooltipRoute<'MultimediaMessageTooltipModal'>,
dispatchFunctions: DispatchFunctions,
bindServerCall: BindServerCall,
) {
const messageID = route.params.item.messageInfo.id;
const threadID = route.params.item.threadInfo.id;
tomekUnsubmitted
Not Done
Inline Actions

This approach seems to be overly complicated. Can't we just write a function that receives messageID, threadID and reaction? This function can be named after an action that is performed, so react or sendReaction.

tomek: This approach seems to be overly complicated. Can't we just write a function that receives…
invariant(messageID, 'messageID should be set');
invariant(threadID, 'threadID should be set');
const callSendReactionMessage = bindServerCall(sendReactionMessage);
const reactionMessagePromise = (async () => {
try {
const result = await callSendReactionMessage(threadID, messageID, '👍');
return {
targetMessageID: messageID,
serverID: result.id,
threadID,
time: result.newMessageInfo.time,
interface: result.interface,
newMessageInfos: [result.newMessageInfo],
};
} catch (e) {
Alert.alert(
tomekUnsubmitted
Not Done
Inline Actions

It sounds risky to show one alert after every failed message. A user might react to a lot of messages in short period and a network issue could result in a lot of alerts being shown. Other apps sometimes choose to silently fail when a reaction fails.

tomek: It sounds risky to show one alert after every failed message. A user might react to a lot of…
ginsuAuthorUnsubmitted
Done
Inline Actions

Totally see your concern here! I spoke to @atul about this today, and we both agreed that we want this to be a temporary solution, but since reaction messages don't have retry behavior we think it is best to show some sort of feedback to the user in case something failed.

ginsu: Totally see your concern here! I spoke to @atul about this today, and we both agreed that we…
'Couldn’t send the reaction',
'Please try again later',
[{ text: 'OK' }],
{
cancelable: false,
tomekUnsubmitted
Not Done
Inline Actions

Is there a reason to set cancelable to false?

tomek: Is there a reason to set cancelable to false?
ginsuAuthorUnsubmitted
Done
Inline Actions

Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by default and can be enabled by providing an optional Options parameter with the cancelable property set to true. Here is more context

I also found the alert on report-utils to follow the same patern

ginsu: Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by…
},
);
throw e;
}
})();
dispatchFunctions.dispatchActionPromise(
sendReactionMessageActionTypes,
reactionMessagePromise,
);
}
export { onPressReact };
View Options

web/chat/composed-message.react.js

  • This file was added.
// @flow
import invariant from 'invariant';
import Alert from 'react-native/Libraries/Alert/Alert';
import {
sendReactionMessage,
sendReactionMessageActionTypes,
} from 'lib/actions/message-actions';
import type { BindServerCall, DispatchFunctions } from 'lib/utils/action-utils';
import type { TooltipRoute } from '../navigation/tooltip.react';
function onPressReact(
route:
| TooltipRoute<'TextMessageTooltipModal'>
| TooltipRoute<'MultimediaMessageTooltipModal'>,
dispatchFunctions: DispatchFunctions,
bindServerCall: BindServerCall,
) {
const messageID = route.params.item.messageInfo.id;
const threadID = route.params.item.threadInfo.id;
tomekUnsubmitted
Not Done
Inline Actions

This approach seems to be overly complicated. Can't we just write a function that receives messageID, threadID and reaction? This function can be named after an action that is performed, so react or sendReaction.

tomek: This approach seems to be overly complicated. Can't we just write a function that receives…
invariant(messageID, 'messageID should be set');
invariant(threadID, 'threadID should be set');
const callSendReactionMessage = bindServerCall(sendReactionMessage);
const reactionMessagePromise = (async () => {
try {
const result = await callSendReactionMessage(threadID, messageID, '👍');
return {
targetMessageID: messageID,
serverID: result.id,
threadID,
time: result.newMessageInfo.time,
interface: result.interface,
newMessageInfos: [result.newMessageInfo],
};
} catch (e) {
Alert.alert(
tomekUnsubmitted
Not Done
Inline Actions

It sounds risky to show one alert after every failed message. A user might react to a lot of messages in short period and a network issue could result in a lot of alerts being shown. Other apps sometimes choose to silently fail when a reaction fails.

tomek: It sounds risky to show one alert after every failed message. A user might react to a lot of…
ginsuAuthorUnsubmitted
Done
Inline Actions

Totally see your concern here! I spoke to @atul about this today, and we both agreed that we want this to be a temporary solution, but since reaction messages don't have retry behavior we think it is best to show some sort of feedback to the user in case something failed.

ginsu: Totally see your concern here! I spoke to @atul about this today, and we both agreed that we…
'Couldn’t send the reaction',
'Please try again later',
[{ text: 'OK' }],
{
cancelable: false,
tomekUnsubmitted
Not Done
Inline Actions

Is there a reason to set cancelable to false?

tomek: Is there a reason to set cancelable to false?
ginsuAuthorUnsubmitted
Done
Inline Actions

Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by default and can be enabled by providing an optional Options parameter with the cancelable property set to true. Here is more context

I also found the alert on report-utils to follow the same patern

ginsu: Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by…
},
);
throw e;
}
})();
dispatchFunctions.dispatchActionPromise(
sendReactionMessageActionTypes,
reactionMessagePromise,
);
}
export { onPressReact };
View Options

web/chat/message-tooltip.css

  • This file was added.
// @flow
import invariant from 'invariant';
import Alert from 'react-native/Libraries/Alert/Alert';
import {
sendReactionMessage,
sendReactionMessageActionTypes,
} from 'lib/actions/message-actions';
import type { BindServerCall, DispatchFunctions } from 'lib/utils/action-utils';
import type { TooltipRoute } from '../navigation/tooltip.react';
function onPressReact(
route:
| TooltipRoute<'TextMessageTooltipModal'>
| TooltipRoute<'MultimediaMessageTooltipModal'>,
dispatchFunctions: DispatchFunctions,
bindServerCall: BindServerCall,
) {
const messageID = route.params.item.messageInfo.id;
const threadID = route.params.item.threadInfo.id;
tomekUnsubmitted
Not Done
Inline Actions

This approach seems to be overly complicated. Can't we just write a function that receives messageID, threadID and reaction? This function can be named after an action that is performed, so react or sendReaction.

tomek: This approach seems to be overly complicated. Can't we just write a function that receives…
invariant(messageID, 'messageID should be set');
invariant(threadID, 'threadID should be set');
const callSendReactionMessage = bindServerCall(sendReactionMessage);
const reactionMessagePromise = (async () => {
try {
const result = await callSendReactionMessage(threadID, messageID, '👍');
return {
targetMessageID: messageID,
serverID: result.id,
threadID,
time: result.newMessageInfo.time,
interface: result.interface,
newMessageInfos: [result.newMessageInfo],
};
} catch (e) {
Alert.alert(
tomekUnsubmitted
Not Done
Inline Actions

It sounds risky to show one alert after every failed message. A user might react to a lot of messages in short period and a network issue could result in a lot of alerts being shown. Other apps sometimes choose to silently fail when a reaction fails.

tomek: It sounds risky to show one alert after every failed message. A user might react to a lot of…
ginsuAuthorUnsubmitted
Done
Inline Actions

Totally see your concern here! I spoke to @atul about this today, and we both agreed that we want this to be a temporary solution, but since reaction messages don't have retry behavior we think it is best to show some sort of feedback to the user in case something failed.

ginsu: Totally see your concern here! I spoke to @atul about this today, and we both agreed that we…
'Couldn’t send the reaction',
'Please try again later',
[{ text: 'OK' }],
{
cancelable: false,
tomekUnsubmitted
Not Done
Inline Actions

Is there a reason to set cancelable to false?

tomek: Is there a reason to set cancelable to false?
ginsuAuthorUnsubmitted
Done
Inline Actions

Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by default and can be enabled by providing an optional Options parameter with the cancelable property set to true. Here is more context

I also found the alert on report-utils to follow the same patern

ginsu: Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by…
},
);
throw e;
}
})();
dispatchFunctions.dispatchActionPromise(
sendReactionMessageActionTypes,
reactionMessagePromise,
);
}
export { onPressReact };
View Options

web/chat/message-tooltip.react.js

  • This file was added.
// @flow
import invariant from 'invariant';
import Alert from 'react-native/Libraries/Alert/Alert';
import {
sendReactionMessage,
sendReactionMessageActionTypes,
} from 'lib/actions/message-actions';
import type { BindServerCall, DispatchFunctions } from 'lib/utils/action-utils';
import type { TooltipRoute } from '../navigation/tooltip.react';
function onPressReact(
route:
| TooltipRoute<'TextMessageTooltipModal'>
| TooltipRoute<'MultimediaMessageTooltipModal'>,
dispatchFunctions: DispatchFunctions,
bindServerCall: BindServerCall,
) {
const messageID = route.params.item.messageInfo.id;
const threadID = route.params.item.threadInfo.id;
tomekUnsubmitted
Not Done
Inline Actions

This approach seems to be overly complicated. Can't we just write a function that receives messageID, threadID and reaction? This function can be named after an action that is performed, so react or sendReaction.

tomek: This approach seems to be overly complicated. Can't we just write a function that receives…
invariant(messageID, 'messageID should be set');
invariant(threadID, 'threadID should be set');
const callSendReactionMessage = bindServerCall(sendReactionMessage);
const reactionMessagePromise = (async () => {
try {
const result = await callSendReactionMessage(threadID, messageID, '👍');
return {
targetMessageID: messageID,
serverID: result.id,
threadID,
time: result.newMessageInfo.time,
interface: result.interface,
newMessageInfos: [result.newMessageInfo],
};
} catch (e) {
Alert.alert(
tomekUnsubmitted
Not Done
Inline Actions

It sounds risky to show one alert after every failed message. A user might react to a lot of messages in short period and a network issue could result in a lot of alerts being shown. Other apps sometimes choose to silently fail when a reaction fails.

tomek: It sounds risky to show one alert after every failed message. A user might react to a lot of…
ginsuAuthorUnsubmitted
Done
Inline Actions

Totally see your concern here! I spoke to @atul about this today, and we both agreed that we want this to be a temporary solution, but since reaction messages don't have retry behavior we think it is best to show some sort of feedback to the user in case something failed.

ginsu: Totally see your concern here! I spoke to @atul about this today, and we both agreed that we…
'Couldn’t send the reaction',
'Please try again later',
[{ text: 'OK' }],
{
cancelable: false,
tomekUnsubmitted
Not Done
Inline Actions

Is there a reason to set cancelable to false?

tomek: Is there a reason to set cancelable to false?
ginsuAuthorUnsubmitted
Done
Inline Actions

Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by default and can be enabled by providing an optional Options parameter with the cancelable property set to true. Here is more context

I also found the alert on report-utils to follow the same patern

ginsu: Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by…
},
);
throw e;
}
})();
dispatchFunctions.dispatchActionPromise(
sendReactionMessageActionTypes,
reactionMessagePromise,
);
}
export { onPressReact };
View Options

web/chat/robotext-message.react.js

Loading...
View Options

web/chat/tooltip-utils.js

Loading...