Page MenuHomePhabricator

[native] introduced more action to tooltip actions
ClosedPublic

Authored by ginsu on Nov 2 2022, 1:10 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Oct 25, 10:43 PM
Unknown Object (File)
Fri, Oct 25, 10:42 PM
Unknown Object (File)
Fri, Oct 25, 10:42 PM
Unknown Object (File)
Fri, Oct 25, 10:42 PM
Unknown Object (File)
Fri, Oct 25, 10:42 PM
Unknown Object (File)
Fri, Oct 25, 10:42 PM
Unknown Object (File)
Fri, Oct 25, 10:42 PM
Unknown Object (File)
Fri, Oct 25, 10:37 PM

Details

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu requested review of this revision.Nov 2 2022, 1:24 PM
atul requested changes to this revision.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?

(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?

ginsu edited the summary of this revision. (Show Details)

addressed atul's comments

atul requested changes to this revision.Nov 7 2022, 7:16 AM

Some questions/suggestions/comments inline

native/navigation/tooltip.react.js
190 ↗(On Diff #18063)

Can we introduce this state in ConnectedTooltip and pass both actionSheetShown and setActionSheetShown into this class component as props?

536 ↗(On Diff #18063)

Maybe getPlatformSpecificButtonIndices?

538–539 ↗(On Diff #18063)

I think we can clean up this comment a bit and be a bit more precise (eg "reversing" what?)

581–589 ↗(On Diff #18063)

Can we define this outside of the call to showActionSheetWithOptions(...) to cut level of indentation?

593–594 ↗(On Diff #18063)

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
native/navigation/tooltip.react.js
190 ↗(On Diff #18063)

I can definitely do that; out of curiosity though, are there any advantages to introducing the state in ConnectedTooltip?

addressed atul's comments

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

atul requested changes to this revision.Nov 7 2022, 9:07 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
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

native/navigation/tooltip.react.js
595 ↗(On Diff #18157)

controls-action-1.png (405×600 px, 46 KB)

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

This revision is now accepted and ready to land.Nov 7 2022, 7:27 PM