Details

Reviewers

atul
rohan

Commits

rCOMM6131061cecca: [native] introduced more action to tooltip actions

Summary

introduced more action to tooltip actions. The more action will only render if a fixed tooltip has more than 3 different actions. This is why only the non-viewer text message has this option. I am aware that some animations are a bit glitchy and there should only be a separator between the more button and the rest of the buttons, but I felt that addressing that was outside the scope of this diff.

Link to @expo/react-native-action-sheet repo

Here are the figma screenshots to reference the designs:

Screen Shot 2022-11-02 at 4.36.42 PM.png (1×476 px, 68 KB)

Screen Shot 2022-11-02 at 4.36.50 PM.png (1×968 px, 155 KB)

Depends on D5523 and D5497
Linear Task: ENG-2037

Test Plan

Please watch the demos to see the changes I made:

Before:

After:
iOS:

Android:

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Nov 2 2022, 1:10 PM

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald TranscriptNov 2 2022, 1:10 PM

Harbormaster completed remote builds in B13143: Diff 18037.Nov 2 2022, 1:24 PM

ginsu requested review of this revision.Nov 2 2022, 1:24 PM

ginsu mentioned this in D5523: [native] introduced flow type for @expo/react-native-action-sheet.Nov 2 2022, 1:25 PM

ginsu edited the summary of this revision. (Show Details)Nov 2 2022, 1:37 PM

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

ginsu added reviewers: atul, rohan.

ginsu added parent revisions: D5523: [native] introduced flow type for @expo/react-native-action-sheet, D5497: [native] Moved fixed tooltip to the bottom of chat window.

ginsu attached a referenced file: F217881: Screen Recording 2022-11-02 at 3.55.53 PM.mov. (Show Details)

ginsu attached a referenced file: F217870: Screen Recording 2022-11-02 at 3.46.37 PM.mov. (Show Details)

ginsu attached a referenced file: F217868: Screen Recording 2022-11-02 at 4.01.29 PM.mov. (Show Details)

ginsu attached a referenced file: F217900: Screen Shot 2022-11-02 at 4.36.50 PM.png. (Show Details)

ginsu attached a referenced file: F217899: Screen Shot 2022-11-02 at 4.36.42 PM.png. (Show Details)

ginsu edited the summary of this revision. (Show Details)Nov 2 2022, 2:08 PM

At a high level this looks good, but the platform-specific indexing login in onPressMore isn't very intuitive without additional context. Could you add some comments explaining what's going on there?

(Thanks for taking to time to prepare all those screenshots + screen recordings! I know that can be tedious..)

native/navigation/tooltip.react.js
535–543 ↗	(On Diff #18037)	This platform-specific indexing stuff isn't super intuitive without additional context. I think it'd be helpful to add some comments here explaining what's going on

This revision now requires changes to proceed.Nov 3 2022, 8:13 AM

At a high level this looks good, but the platform-specific indexing login in onPressMore isn't very intuitive without additional context. Could you add some comments explaining what's going on there?

Actually I'd personally prefer pulling the platform-specific logic outside of onPressMore into a separate helper function... would you be open to exploring that if it's not too involved?

addressed atul's comments

edited comment

Harbormaster completed remote builds in B13160: Diff 18062.Nov 3 2022, 1:02 PM

Harbormaster completed remote builds in B13161: Diff 18063.Nov 3 2022, 1:05 PM

rohan accepted this revision.Nov 7 2022, 5:38 AM

Some questions/suggestions/comments inline

native/navigation/tooltip.react.js
190	Can we introduce this state in `ConnectedTooltip` and pass both `actionSheetShown` and `setActionSheetShown` into this class component as `prop`s?
536	Maybe `getPlatformSpecificButtonIndices`?
538–539	I think we can clean up this comment a bit and be a bit more precise (eg "reversing" what?)
581–589	Can we define this outside of the call to `showActionSheetWithOptions(...)` to cut level of indentation?
593–594	Can we clean this comment up a bit to explain the purpose of the function (which is to handle platform specific differences) at a high level. Details of the differences can be explained within the implementation of the function.

This revision now requires changes to proceed.Nov 7 2022, 7:16 AM

ginsu added inline comments.Nov 7 2022, 7:19 AM

native/navigation/tooltip.react.js
190	I can definitely do that; out of curiosity though, are there any advantages to introducing the state in `ConnectedTooltip`?

ginsu mentioned this in rCOMM777c5f57ace0: [native] introduced flow type for @expo/react-native-action-sheet.Nov 7 2022, 7:22 AM

addressed atul's comments

Harbormaster failed remote builds in B13220: Diff 18144!Nov 7 2022, 8:51 AM

ginsu added inline comments.Nov 7 2022, 8:59 AM

native/navigation/tooltip.react.js
657–659 ↗	(On Diff #18144)	Would love to better understand what the advantages to introducing the state in `ConnectedTooltip` and passing it down as a prop instead of just having the state in the main component

Harbormaster completed remote builds in B13220: Diff 18144.Nov 7 2022, 9:01 AM

I think if we change the comments this should be good to land

native/navigation/tooltip.react.js
534–536 ↗	(On Diff #18144)	How about We're reversing `options` to populate the action sheet from bottom to top instead of the default (top to bottom) ordering.
592–593 ↗	(On Diff #18144)	I don't think we really need this comment since the function name is more descriptive now.
598 ↗	(On Diff #18144)	How about The "Cancel" action is iOS-specific Can you clarify what you mean by "almost always ios specific?"

This revision now requires changes to proceed.Nov 7 2022, 9:07 AM

atul added inline comments.Nov 7 2022, 9:09 AM

native/navigation/tooltip.react.js
657–659 ↗	(On Diff #18144)	Personally think it's cleaner/simpler to have all the "state stuff" handled in the outer `Connected` component

updated comments

ginsu added inline comments.Nov 7 2022, 10:26 AM

native/navigation/tooltip.react.js
595 ↗	(On Diff #18157)	Example of action sheet on iOS vs Android, found on source. Hoping this provides more context to why the indices need to be different for each platform