Details

Reviewers

atul
rohan
tomek

Commits

rCOMMf482e4b54785: [native] switched fixed tooltip component from a vertical layout to horizontal

Summary

Tooltips that have a fixed location now have a horizontal layout, don't render the triangles, and have a minWidth of the screen size with 8px of padding on each side. Also created a fixedTooltipHeight variable since the tooltip height for fixed tooltips will always be the same, and using inspector found out that the height of the new tooltip is 53px.

Screen Shot 2022-10-17 at 5.18.58 PM.png (1×1 px, 762 KB)

Depends on D5468
Linear Task: ENG-2037
Design: Figma

Test Plan

Please review the screenshots below to see the changes I made, before and after:

Create Thread Case:

Screen Shot 2022-10-17 at 10.28.42 AM.png (1×1 px, 615 KB)

Open Thread Case:

Screen Shot 2022-10-17 at 11.15.40 AM.png (1×1 px, 634 KB)

Inspector View:

Screen Shot 2022-10-17 at 10.10.42 AM.png (1×958 px, 526 KB)

Video Demo:

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Oct 17 2022, 7:27 AM

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald TranscriptOct 17 2022, 7:27 AM

Harbormaster completed remote builds in B12829: Diff 17614.Oct 17 2022, 7:35 AM

ginsu requested review of this revision.Oct 17 2022, 7:35 AM

ginsu edited the summary of this revision. (Show Details)Oct 17 2022, 8:17 AM

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

ginsu added reviewers: atul, rohan.

ginsu attached a referenced file: F202159: Screen Recording 2022-10-17 at 11.16.30 AM.mov. (Show Details)

ginsu attached a referenced file: F202158: Screen Shot 2022-10-17 at 11.15.40 AM.png. (Show Details)

ginsu attached a referenced file: F202157: Screen Shot 2022-10-17 at 10.28.42 AM.png. (Show Details)

ginsu attached a referenced file: F202082: Screen Shot 2022-10-17 at 10.10.42 AM.png. (Show Details)

ginsu attached a referenced file: F202081: Screen Shot 2022-10-15 at 2.51.27 PM.png. (Show Details)

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

removed extra console.log

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

ginsu added a parent revision: D5374: [native] added icons to tooltip item component.

Harbormaster completed remote builds in B12833: Diff 17618.Oct 17 2022, 8:27 AM

Really cool, pulled it down locally and everything looks good to me! Just a question below

native/navigation/tooltip.react.js
524 ↗	(On Diff #17618)	Since you removed the triangle style, does anything in this function change? I don't have too much context on this so just wondering if removing the triangle style impacts the calculation based on the in-line comment here.

Looks close, left a few comments inline

native/chat/text-message-tooltip-modal.react.js
59–64 ↗	(On Diff #17618)	Personally think we should keep the old copy even if the designs may differ. (CC @ashoat)
native/navigation/tooltip.react.js
311–317 ↗	(On Diff #17618)	Looks like `style.minWidth` is the same regardless of `extraLeftSpace < extraRightSpace`... can we just pull it out of the `if/else` block?
499–512 ↗	(On Diff #17618)	These changes are clean
524 ↗	(On Diff #17618)	Yeah let's make sure the calculation here is correct

This revision now requires changes to proceed.Oct 17 2022, 10:32 AM

ashoat added inline comments.Oct 17 2022, 10:34 AM

native/chat/text-message-tooltip-modal.react.js
59–64 ↗	(On Diff #17618)	I think we're good to change copy

Also don't think it's a huge deal, but you could've probably split this diff even further into

Remove triangle stuff (and make any necessary changes to tooltipHeight(...))
Change layout from vertical to horizontal

Personally think it's fine to leave this diff as is, but just a thought for next time.

addressed feedback

ginsu edited the summary of this revision. (Show Details)Oct 17 2022, 2:36 PM

ginsu attached a referenced file: F202391: Screen Shot 2022-10-17 at 5.18.58 PM.png. (Show Details)

Harbormaster completed remote builds in B12841: Diff 17629.Oct 17 2022, 2:45 PM

switched tooltipHeight from function to constant

Harbormaster completed remote builds in B12842: Diff 17630.Oct 17 2022, 3:16 PM

atul added a reviewer: tomek.Oct 19 2022, 10:23 AM

Looks good!

~~(Make sure to separate out copy changes before landing!)~~

edit: jk it looks like it was discussed

native/chat/text-message-tooltip-modal.react.js
57–64 ↗	(On Diff #17630)	I think this is an unrelated change? Changing the copy here should be in a separate diff

This revision is now accepted and ready to land.Oct 19 2022, 10:25 AM

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

native/chat/text-message-tooltip-modal.react.js
57–64 ↗	(On Diff #17630)	I'm down with the copy change but it seems like we could dedup these now that they have the same title. Makes sense to handle that in a separate diff

ginsu added a child revision: D5451: [native] deduped open thread and create thread option.Oct 20 2022, 2:33 PM

rebase

Harbormaster completed remote builds in B12996: Diff 17855.Oct 24 2022, 8:42 AM

updated diff to consider ThreadSettingsMemberTooltipModal and RelationshipListItemTooltipModal

removed tooltipHeight in exports since it is no longer being used in other files

Harbormaster completed remote builds in B13036: Diff 17905.Oct 25 2022, 12:20 PM

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

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

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

ginsu added 1 blocking reviewer(s): atul.

ginsu added a parent revision: D5468: [native] introduced fixed location option for tooltip toolbar.

This revision now requires review to proceed.Oct 25 2022, 12:26 PM

ginsu attached a referenced file: F207760: Screen Recording 2022-10-25 at 3.04.48 PM.mov. (Show Details)Oct 25 2022, 12:27 PM

Harbormaster completed remote builds in B13038: Diff 17907.Oct 25 2022, 12:30 PM

Super close, minor nit/question inline

native/chat/multimedia-message.react.js
23	Super minor nit, but let's make "tooltip" one word to remain consistent with the rest of the codebase. Should be something like: fixedTooltipHeight
native/navigation/tooltip.react.js
580	Why are we deleting this comment? Should it be replaced with a comment that explains "the new math?"

This revision now requires changes to proceed.Oct 26 2022, 12:22 PM

addressed atul's comments

Harbormaster completed remote builds in B13078: Diff 17955.Oct 27 2022, 8:29 AM

Let's update the comment at tooltip.react.js:618 to correctly reflect the "math" below and this should be good to land.

native/navigation/tooltip.react.js
618 ↗	(On Diff #17955)	We should update this comment to reflect the "math" below

This revision now requires changes to proceed.Oct 27 2022, 9:41 AM

Spoke to atul, for now the math of tooltipHeight is outside the scope of this diff, made a linear task to track this issue

atul accepted this revision.Oct 27 2022, 12:01 PM

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

rebase

Harbormaster completed remote builds in B13264: Diff 18199.Nov 8 2022, 7:54 AM

Closed by commit rCOMMf482e4b54785: [native] switched fixed tooltip component from a vertical layout to horizontal (authored by ginsu). · Explain WhyNov 8 2022, 7:58 AM

This revision was automatically updated to reflect the committed changes.

ginsu added a commit: rCOMMf482e4b54785: [native] switched fixed tooltip component from a vertical layout to horizontal.

[native] switched fixed tooltip component from a vertical layout to horizontal
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 17907

native/chat/multimedia-message-tooltip-modal.react.js

native/chat/multimedia-message.react.js

native/chat/robotext-message-tooltip-modal.react.js

native/chat/robotext-message.react.js

native/chat/text-message-tooltip-modal.react.js

native/chat/text-message.react.js

native/navigation/app-navigator.react.js

native/navigation/tooltip.react.js

[native] switched fixed tooltip component from a vertical layout to horizontalClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 17907

native/chat/multimedia-message-tooltip-modal.react.js

native/chat/multimedia-message.react.js

native/chat/robotext-message-tooltip-modal.react.js

native/chat/robotext-message.react.js

native/chat/text-message-tooltip-modal.react.js

native/chat/text-message.react.js

native/navigation/app-navigator.react.js

native/navigation/tooltip.react.js

[native] switched fixed tooltip component from a vertical layout to horizontal
ClosedPublic
Actions

Revision Contents
Changeset List