Page MenuHomePhabricator
Differential D5452

[native] removed tooltip location logic
AbandonedPublic
Actions

Authored by ginsu on Oct 20 2022, 3:12 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 28, 11:36 PM
Unknown Object (File)
Thu, Nov 28, 11:36 PM
Unknown Object (File)
Mon, Nov 25, 9:02 PM
Unknown Object (File)
Mon, Nov 25, 3:45 AM
Unknown Object (File)
Sun, Nov 24, 11:00 PM
Unknown Object (File)
Nov 22 2024, 10:35 AM
Unknown Object (File)
Nov 22 2024, 5:39 AM
Unknown Object (File)
Oct 25 2024, 10:43 PM
View All Files
Subscribers
• abosh
ashoat
atul
tomek

Details

Reviewers
atul
rohan
ashoat
Summary

Will not land this diff until the next diff in the stack is accepted

removed the tooltip location logic since eventually, the tooltip toolbar will always be at the bottom of the chat window

Depends on D5451
Linear Task: ENG-2037

Test Plan

Please review the video demo to see that the tooltip component is now always going below the message:

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu created this revision.Oct 20 2022, 3:12 PM
Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald TranscriptOct 20 2022, 3:12 PM
ginsu edited the summary of this revision. (Show Details)Oct 20 2022, 3:16 PM
ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: atul, rohan.
ginsu added a parent revision: D5451: [native] deduped open thread and create thread option.
ginsu attached a referenced file: F205235: Screen Recording 2022-10-20 at 6.13.46 PM.mov. (Show Details)
ginsu edited the summary of this revision. (Show Details)
ginsu edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B12954: Diff 17795.Oct 20 2022, 3:20 PM
ginsu requested review of this revision.Oct 20 2022, 3:20 PM
ashoat added inline comments.Oct 20 2022, 7:37 PM
native/navigation/tooltip.react.js
315 ↗(On Diff #17795)

I wonder if it's worth keeping this feature in Tooltip just in case we want to use it in the future. @atul, what do you think?

ginsu added a child revision: D5459: [native] moved tooltip toolbar to the bottom of the chat window.Oct 21 2022, 12:45 PM
atul added a comment.EditedOct 24 2022, 7:00 AM

@atul, what do you think?

I don't think it hurts to keep this positioning logic around. It might be helpful if we use Tooltip for the row of emoji reactions?

It looks like we have designs in Figma for the row of emoji to appear both above and below the message bubble:

aaac82.png (1×1 px, 178 KB)

ashoat requested changes to this revision.Oct 24 2022, 7:59 AM

Cool, agree

This revision now requires changes to proceed.Oct 24 2022, 7:59 AM
ginsu updated this revision to Diff 17857.Oct 24 2022, 8:31 AM

rebase

ginsu planned changes to this revision.Oct 24 2022, 8:35 AM

planning changes for addressing feedback

Harbormaster completed remote builds in B12998: Diff 17857.Oct 24 2022, 8:44 AM
ginsu abandoned this revision.Oct 24 2022, 9:15 AM

after speaking to @atul we came up with a better strategy of instead of removing the location logic, adding a fixed option to the location logic that fixes the toolbar to the bottom of the chat window

Revision Contents
Changeset List

PathSize
native/
chat/
5 lines
5 lines
5 lines
navigation/
43 lines

Diff 17857

View Options

native/chat/multimedia-message.react.js

Loading...
View Options

native/chat/robotext-message.react.js

Loading...
View Options

native/chat/text-message.react.js

Loading...
View Options

native/navigation/tooltip.react.js

Loading...