Page MenuHomePhabricator
Differential D5381

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

Authored by ginsu on Oct 17 2022, 7:27 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 5, 7:19 AM
Unknown Object (File)
Tue, Nov 5, 7:19 AM
Unknown Object (File)
Sat, Nov 2, 6:37 PM
Unknown Object (File)
Sat, Nov 2, 6:29 PM
Unknown Object (File)
Sat, Nov 2, 9:59 AM
Unknown Object (File)
Sat, Nov 2, 7:09 AM
Unknown Object (File)
Fri, Oct 25, 10:43 PM
Unknown Object (File)
Fri, Oct 25, 10:43 PM
View All Files
Subscribers
• abosh
ashoat
atul
tomek

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)
ginsu updated this revision to Diff 17618.Oct 17 2022, 8:19 AM

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
rohan accepted this revision.Oct 17 2022, 9:10 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.

atul requested changes to this revision.Oct 17 2022, 10:32 AM

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

atul added a comment.Oct 17 2022, 10:35 AM

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

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

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

ginsu updated this revision to Diff 17629.Oct 17 2022, 2:34 PM
ginsu edited the summary of this revision. (Show Details)

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
ginsu updated this revision to Diff 17630.Oct 17 2022, 3:03 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
atul accepted this revision.EditedOct 19 2022, 10:25 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
ginsu updated this revision to Diff 17855.Oct 24 2022, 8:30 AM

rebase

Harbormaster completed remote builds in B12996: Diff 17855.Oct 24 2022, 8:42 AM
ginsu updated this revision to Diff 17905.Oct 25 2022, 12:07 PM

updated diff to consider ThreadSettingsMemberTooltipModal and RelationshipListItemTooltipModal

ginsu updated this revision to Diff 17907.Oct 25 2022, 12:17 PM

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
atul requested changes to this revision.Oct 26 2022, 12:22 PM

Super close, minor nit/question inline

native/chat/multimedia-message.react.js
23 ↗(On Diff #17907)

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 ↗(On Diff #17907)

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
ginsu updated this revision to Diff 17955.Oct 27 2022, 8:16 AM

addressed atul's comments

Harbormaster completed remote builds in B13078: Diff 17955.Oct 27 2022, 8:29 AM
atul requested changes to this revision.Oct 27 2022, 9:41 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
ginsu requested review of this revision.Oct 27 2022, 11:51 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
ginsu updated this revision to Diff 18199.Nov 8 2022, 7:40 AM

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.

Revision Contents
Changeset List

Diff 17629

View Options

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

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

native/navigation/tooltip.react.js

Loading...