Paths

Table of Contentst

Differential D5452

[native] removed tooltip location logic
AbandonedPublic
Actions

Authored by ginsu on Oct 20 2022, 3:12 PM.

Tags

None

Referenced Files

	F3364173: D5452.diff
	Mon, Nov 25, 3:45 AM

	F3362527: D5452.id17795.diff
	Sun, Nov 24, 11:00 PM

	Unknown Object (File)
	Fri, Nov 22, 10:35 AM

	Unknown Object (File)
	Fri, Nov 22, 5:39 AM

	Unknown Object (File)
	Oct 25 2024, 10:43 PM

	Unknown Object (File)
	Oct 25 2024, 10:43 PM

	Unknown Object (File)
	Oct 25 2024, 10:42 PM

	Unknown Object (File)
	Oct 25 2024, 10:37 PM

Subscribers

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

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, 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)

Cool, agree

This revision now requires changes to proceed.Oct 24 2022, 7:59 AM

rebase

planning changes for addressing feedback

Harbormaster completed remote builds in B12998: Diff 17857.Oct 24 2022, 8:44 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

Path

Size

native/

chat/

multimedia-message.react.js

5 lines

robotext-message.react.js

5 lines

text-message.react.js

5 lines

navigation/

tooltip.react.js

43 lines

Diff 17857

native/chat/multimedia-message.react.js

Loading...

native/chat/robotext-message.react.js

Loading...

native/chat/text-message.react.js

Loading...

native/navigation/tooltip.react.js

Loading...