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.
Details
Please review the screenshots below to see the changes I made, before and after:
Create Thread Case:
Open Thread Case:
Inspector View:
Video Demo:
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
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 |
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.
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 | I think this is an unrelated change? Changing the copy here should be in a separate diff |
native/chat/text-message-tooltip-modal.react.js | ||
---|---|---|
57–64 | 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 |
updated diff to consider ThreadSettingsMemberTooltipModal and RelationshipListItemTooltipModal
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?" |
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 |
Spoke to atul, for now the math of tooltipHeight is outside the scope of this diff, made a linear task to track this issue