Page MenuHomePhabricator

[native] Create the toggle pin modal
ClosedPublic

Authored by rohan on Apr 25 2023, 5:56 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, May 1, 1:36 PM
Unknown Object (File)
Wed, Apr 10, 8:48 PM
Unknown Object (File)
Wed, Apr 3, 10:16 PM
Unknown Object (File)
Wed, Apr 3, 10:16 PM
Unknown Object (File)
Wed, Apr 3, 10:16 PM
Unknown Object (File)
Wed, Apr 3, 10:16 PM
Unknown Object (File)
Wed, Apr 3, 10:16 PM
Unknown Object (File)
Wed, Apr 3, 10:16 PM
Subscribers

Details

Summary

When pressing the icon in the tooltip, a modal should pop up on the screen allowing the user to pin / unpin a message. This diff handles the modal creation, but to make it easier to review, the
actual rendering of the message in the modal will be in the next diff following this one.

I placed the modal in app-navigator.react.js since the TextMessage component requires the OverlayContext to be set, so when rendering the Message > TextMessage inside of this modal in the next diff,
we won't have the invariant requiring this context be thrown.

Note: The approach here on native follows closely the approach for web here: https://phab.comm.dev/D7309

Depends on D7606

Test Plan

Confirm that the modal appears (not rendering the message yet) for messages and multimedia, both that should be pinned and unpinned.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
native/chat/message-result.react.js
15 ↗(On Diff #25659)

Resolved in the next diff once MessageResult is built out, as mentioned in the description

native/chat/toggle-pin-modal.react.js
69–97 ↗(On Diff #25659)

Ideally, since the return statements are the same, I'd have preferred just to return once, but without this I was running into Flow errors since ChatMultimediaMessageInfoItem has some more properties that ChatTextMessageInfoItemWithHeight does not have.

Cannot create `MessageResult` element because  possibly missing property `contentWidth` in `ChatTextMessageInfoItemWithHeight` [1] is incompatible with  number [2] in property `contentWidth` of property `item`.

Not sure if there is a better way to handle this?

rohan requested review of this revision.Apr 25 2023, 6:14 AM

Copying comments over

native/chat/message-result.react.js
15 ↗(On Diff #25880)

Resolved in the next diff once MessageResult is built out, as mentioned in the description

native/chat/toggle-pin-modal.react.js
69–97 ↗(On Diff #25880)

Ideally, since the return statements are the same, I'd have preferred just to return once, but without this I was running into Flow errors since ChatMultimediaMessageInfoItem has some more properties that ChatTextMessageInfoItemWithHeight does not have.

Cannot create `MessageResult` element because  possibly missing property `contentWidth` in `ChatTextMessageInfoItemWithHeight` [1] is incompatible with  number [2] in property `contentWidth` of property `item`.
Not sure if there is a better way to handle this?

Replace the TextMessageTooltipModal overlay with the toggle pin modal (and also the
MultimediaMessageTooltipModal, but this one is less problematic).

Wrote a comment explaining why following a sync with Ashoat, but dismissing the tooltip overlay prior to
displaying the toggle pin modal was essential in preventing unwanted behavior with certain text messages
not showing up because they were being 'hidden' while the tooltip overlay was displayed.

Separate multimedia tooltip navigation logic out into the correct file

Copying comment over from previous revisions

native/chat/toggle-pin-modal.react.js
69–97 ↗(On Diff #26021)

Ideally, since the return statements are the same, I'd have preferred just to return once, but without this I was running into Flow errors since ChatMultimediaMessageInfoItem has some more properties that ChatTextMessageInfoItemWithHeight does not have.

Cannot create `MessageResult` element because  possibly missing property `contentWidth` in `ChatTextMessageInfoItemWithHeight` [1] is incompatible with  number [2] in property `contentWidth` of property `item`.

Not sure if there is a better way to handle this?

atul requested changes to this revision.May 3 2023, 7:43 AM

Left some high level feedback. Adding @ashoat as blocking to take a look at the navigation stuff (I'm not super familiar with how navigation works, but it looks right?)

native/chat/message-result.react.js
17 ↗(On Diff #26021)

I think you could just return null instead of <View />?

Though it does look like there's another example of this in the codebase:

d32608.png (402×1 px, 79 KB)

(cc @ashoat)

native/chat/multimedia-message-tooltip-modal.react.js
42 ↗(On Diff #26021)

Could we early-exit here to reduce indentation?

Like

if (routeName !== MultimediaMessageTooltipModalRouteName) {
  return;
}
navigation.dispatch....
native/chat/text-message-tooltip-modal.react.js
115 ↗(On Diff #26021)

I'd add a newline after this to improve readability.

native/chat/toggle-pin-modal.react.js
49 ↗(On Diff #26021)

This is 82 characters wide, can we split to multiple lines

58 ↗(On Diff #26021)

This is 162 characters wide, can we split to multiple lines

69–97 ↗(On Diff #26021)

Spent 5ish min and couldn't figure out anything simpler. Could pull out and then include/exclude some top-level fields, but ran into some issues with the "insides" of the messageInfo field. CC @ashoat who might have some better ideas.

Given the way things are now, can we structure as if/else if/else?

101–110 ↗(On Diff #26021)

Could we define this outside of the onPress?

193 ↗(On Diff #26021)

Is there a variable from design system we can use here?

This revision now requires changes to proceed.May 3 2023, 7:43 AM
ashoat requested changes to this revision.May 3 2023, 7:49 AM

Generally agree with @atul's feedback. The ReactNav stuff looks generally good here barring the wrong ParamList being used

native/chat/multimedia-message-tooltip-modal.react.js
43–49 ↗(On Diff #26021)

This is right – context here, and we already tried navigation.replace (doesn't work here since this isn't a stack navigation prop, it's an overlay navigation prop)

native/chat/text-message-tooltip-modal.react.js
96–114 ↗(On Diff #26021)

Can we avoid copy-pasting this? Maybe it should be factored out into a hook?

native/chat/toggle-pin-modal.react.js
69–97 ↗(On Diff #26021)

Yeah, unfortunately this is normal... you can add a // This conditional is for Flow comment or something for clarity

native/navigation/route-names.js
108 ↗(On Diff #26021)

This is the wrong ParamList for this. It should go in OverlayParamList

rohan marked 11 inline comments as done.

Address feedback

ashoat requested changes to this revision.May 6 2023, 1:49 AM

Thought a little bit more carefully about the ReactNav stuff and have just one more round of feedback (hopefully), all regarding the new useNavigateToPinModal hook. Sorry I didn't call this out earlier!

native/utils/toggle-pin-utils.js
22 ↗(On Diff #26134)

Typo here (Tetx)

26 ↗(On Diff #26134)

Can we try to avoid crashing if visibleOverlays is empty? It would be an unusual edge case, but I could imagine the modal getting dismissed right before the pin action is selected somehow

26 ↗(On Diff #26134)

I noticed you're no longer checking the routeName. This is somewhat concerning... you could for instance imagine a scenario where somebody quickly presses Copy (which pulls up an ActionResultModal overlay) and then presses Pin (which would replace the ActionResultModal overlay, but keep the tooltip overlay)

I think we should make this look for the most recent entry in visibleOverlays that has a valid tooltip routeName, and then use its routeKey. If we can't find one, I think we should just do a navigation.navigate.

Regarding checking the routeName, rather than explicitly listing the three tooltip route names, I think it would be good to create an array of all three in native/navigation/route-names.js next to where the MessageTooltipRouteNames type is defined, and then import that here for the check.

27 ↗(On Diff #26134)

If routeKey is undefined here, I think we should avoid calling StackActions.replace, and instead should call navigation.navigate. Looking at the ReactNav docs, if for some reason there are no active overlays when this is called, we risk a situation where we replace the bottom route, which contains the whole app navigation structure

This revision now requires changes to proceed.May 6 2023, 1:49 AM

Added optional chaining to visibleOverlays.slice to ensure that in the potential edge case where it is empty/undefined, the app will not crash. This was tested by forcing .slice(-1) to be called on a null value, and ensuring that navigate was called instead of replace, as before this change it would have crashed.

Created a PinnableMessageTooltipRouteNames array that contains TextMessageTooltipRouteName and MultimediaMessageTooltipRouteName, and I check if the current routeName is present in the array - if it is, I call replace, otherwise just navigate. I ran a console.log(PinnableMessageTooltipRouteNames) to verify that the array was created correctly: ["TextMessageTooltipModal", "MultimediaMessageTooltipModal"].

In the event that routeKey is undefined, I also call navigate instead of replace (just merged the two checks into one if statement). This change was tested by ensuring the replace works fine with current behavior, and then I set routeKey to null in the hook and noticed that navigate got called instead, verified by the fact that the text message tooltip had not been replaced and was still displayed once the pin modal was closed.

I’m confused how the optional chaining works here – if ?.slice(-1) is called on an undefined value and returns undefined, won’t you still be indexing on undefined (undefined[0])? Also note that the concern I shared was about the array being empty, whereas you seem to explored the possibility of the array being null

Accepting to unblock, but please make sure you’ve tested and can confirm that it all works! Separately, please address inline comments before landing

native/utils/toggle-pin-utils.js
49 ↗(On Diff #26140)

Nit: might be better to handle the less-indented case first

50 ↗(On Diff #26140)

Nit: can we define the params once instead of copy-pasting them?

I’m confused how the optional chaining works here – if ?.slice(-1) is called on an undefined value and returns undefined, won’t you still be indexing on undefined (undefined[0])?

Optional chaining should short-circuit and evaluate to undefined, so slice shouldn't be reached.

Screenshot 2023-05-06 at 8.30.36 AM.png (1×1 px, 193 KB)

Also note that the concern I shared was about the array being empty, whereas you seem to explored the possibility of the array being null

The .slice() method called on an empty array will return an empty array, and in javascript accessing a non-existent index returns undefined, so worst case mostRecentOverlay should just default to undefined, which won't crash

atul added inline comments.
native/chat/message-result.react.js
17 ↗(On Diff #26197)

Can we not return null?

native/chat/toggle-pin-modal.react.js
190 ↗(On Diff #26197)

Part of using a variable is to make sure things appear as expected in both light and dark mode. Does whiteText appear as expected on both?

This revision is now accepted and ready to land.May 8 2023, 7:14 AM
native/chat/message-result.react.js
17 ↗(On Diff #26197)

Don't think it really matters at this point given it's expanded later in the stack and Rohan is trying to land all of this today

native/chat/toggle-pin-modal.react.js
190 ↗(On Diff #26197)

@rohan please make sure to confirm everything looks good in both light mode and dark mode before landing!

native/chat/message-result.react.js
17 ↗(On Diff #26197)

Sorry, I didn't respond to your comment earlier when addressing the feedback. The next diff replaces this with the actual component

native/chat/toggle-pin-modal.react.js
190 ↗(On Diff #26197)

Thanks for calling this out, you're right whiteText is not visible during light mode. Let me look at an alternative (could just use the same color as modalConfirmationText, namely specifically 'panelBackgroundLabel'

Fix text colors so they are visible on both light and dark mode

Is panelBackgroundLabel actually the right color to use here per the designs?

If the designs are in dark mode, then we should make sure we use the exact right color that the designs have for dark mode.

Is panelBackgroundLabel actually the right color to use here per the designs?

If the designs are in dark mode, then we should make sure we use the exact right color that the designs have for dark mode.

Just realized modalButtonLabel is the correct color in dark mode, and adapts to light mode well. I'll update again

Use modalButtonLabel, the correct shade in dark mode and adapts to light
mode

native/chat/toggle-pin-modal.react.js
158 ↗(On Diff #26211)

Sorry I'm just noticing this now, but can we use the modal family here instead of inserting a rando color from the panel family? Looks like modalBackgroundLabel is the same in dark mode, but differs in light mode. If it doesn't look crazy in light mode, I think we can go with that

Use modalBackgroundLabel instead, tested on light and dark mode

This revision was automatically updated to reflect the committed changes.