[native] introduced fixed location option for tooltip toolbar
ClosedPublic
Actions

Authored by ginsu on Oct 24 2022, 9:31 AM.

Details

Reviewers

atul
rohan
ashoat

Commits

rCOMM76c7b716959d: [native] introduced fixed location option for tooltip toolbar

Summary

Will not land diff until all diffs in stack are accepted

introduced fixed as a loction option for the tooltip toolbar. The following tooltips will be using the fixed option: multimedia-message.react, robotext-message.react, and text-message.react

Linear Task: ENG-2037

Test Plan

Please watch the demo to see that multimedia-message.react, robotext-message.react, and text-message.react now only get the tooltip placed below the message, this is to be expected and will be addressed later:

Diff Detail

Repository

rCOMM Comm

Branch

eng-2037 (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Oct 24 2022, 9:31 AM

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald TranscriptOct 24 2022, 9:31 AM

ginsu edited the summary of this revision. (Show Details)Oct 24 2022, 9:35 AM

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

ginsu added reviewers: atul, rohan.

ginsu attached a referenced file: F206858: Screen Recording 2022-10-24 at 12.32.30 PM.mov. (Show Details)

ginsu mentioned this in D5459: [native] moved tooltip toolbar to the bottom of the chat window.

Harbormaster completed remote builds in B13007: Diff 17866.Oct 24 2022, 9:43 AM

ginsu requested review of this revision.Oct 24 2022, 9:43 AM

ashoat added inline comments.Oct 24 2022, 10:19 AM

native/chat/multimedia-message.react.js
146	Doesn't seem like we need a variable for this anymore. Let's move it to where it's used
native/navigation/tooltip.react.js
318	Where does 32 come from?

ginsu added inline comments.Oct 24 2022, 10:49 AM

native/navigation/tooltip.react.js
318	it's the fixed margin to the bottom of the chat window- i will pull out this number into a variable so that it is more descriptive

Sending back to your queue while you address feedback

This revision now requires changes to proceed.Oct 24 2022, 10:58 AM

addressed ashoat's comments

ginsu edited the test plan for this revision. (Show Details)Oct 24 2022, 10:59 AM

ginsu attached a referenced file: F207125: Screen Shot 2022-10-24 at 1.58.10 PM.png. (Show Details)

Harbormaster completed remote builds in B13018: Diff 17877.Oct 24 2022, 11:12 AM

planning changes to fixedMargin variable

Would love to understand the 32 number better! I have a suspicion that we shouldn't be hardcoding that... eg. if it's the height of some React Nav component, I think there's an API for that... or if it's the height of one of our components, we should probably keep that constant in the same file as that component (if not something better like what React Nav does)

In D5468#161710, @ashoat wrote:

Would love to understand the 32 number better! I have a suspicion that we shouldn't be hardcoding that... eg. if it's the height of some React Nav component, I think there's an API for that... or if it's the height of one of our components, we should probably keep that constant in the same file as that component (if not something better like what React Nav does)

Will address this in a future diff

just introducing fixed option

ginsu edited the summary of this revision. (Show Details)Oct 25 2022, 7:46 AM

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

ginsu added 1 blocking reviewer(s): atul.

ginsu attached a referenced file: F207584: Screen Recording 2022-10-25 at 10.44.24 AM.mov. (Show Details)

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

Harbormaster completed remote builds in B13031: Diff 17898.Oct 25 2022, 7:55 AM

ashoat accepted this revision.Oct 25 2022, 7:59 AM

ginsu added a child revision: D5381: [native] switched fixed tooltip component from a vertical layout to horizontal.Oct 25 2022, 12:26 PM

atul accepted this revision.Oct 26 2022, 12:55 PM

This revision is now accepted and ready to land.Oct 26 2022, 12:55 PM