Page MenuHomePhabricator

[web] fix message action tooltip placement for messages with inline engagement
ClosedPublic

Authored by ginsu on Sep 7 2023, 7:46 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 20, 5:15 PM
Unknown Object (File)
Wed, Nov 20, 5:15 PM
Unknown Object (File)
Wed, Nov 20, 5:15 PM
Unknown Object (File)
Wed, Nov 20, 5:15 PM
Unknown Object (File)
Wed, Nov 20, 5:14 PM
Unknown Object (File)
Nov 5 2024, 10:13 AM
Unknown Object (File)
Oct 29 2024, 12:20 PM
Unknown Object (File)
Oct 25 2024, 8:05 AM

Details

Summary

This diff addresses https://linear.app/comm/issue/ENG-4814/weird-message-action-placement-on-messages-with-engagements

As I was working on improving tooltips during the inline engagement redesign, I found this old linear comment to be helpful for context:
https://linear.app/comm/issue/ENG-1575#comment-ef9727c3

and specifically for this particular bug:

When we display the inline sidebar, we want to avoid covering the sidebar with the message. To achieve this, we avoid rendering the tooltip below the bottom edge of the message (as the InlineSidebar can have different width and may be much wider, that the source message). Because tooltip in this case is much higher than robotext message, it cannot be displayed in the LEFT and LEFT_TOP positions (as the bottom part of tooltip could cover the sidebar), so it’s displayed in LEFT_BOTTOM, what means it’s aligned to the bottom edge of the message.It makes the tooltip appear a bit higher and make it look like the timestamp is on the same height as the message.

Test Plan

Please see the demo videos below

Before:

After:

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

web/utils/tooltip-action-utils.js
51 ↗(On Diff #30837)

This param is now not being used anymore by any tooltip, so I thought it would be best to just get rid of it. This should also now make the useTooltip hook 100% "tooltip-agnostic"

web/utils/tooltip-utils.js
116 ↗(On Diff #30837)

willCoverSidebarOnTopSideways will always now be false and so now this condition will just always be true and is now not necessary

132 ↗(On Diff #30837)

willCoverSidebarInTheMiddleSideways will always now be false and so now this condition will just always be true and is now not necessary

143 ↗(On Diff #30837)

preventDisplayingBelowSource will always now be false and so now this condition will just always be true and is now not necessary

web/utils/tooltip-utils.test.js
143–178 ↗(On Diff #30837)

These tests are no longer relevant since we are not concerned about not covering an element below source anymore

159 ↗(On Diff #30837)

By removing the preventDisplayingBelowSource param, findTooltipPosition was now finding a valid available position for the tooltip so just changed the sourcePositionInfo to top right so that findTooltipPosition would just return the default position and this test would still pass

ginsu requested review of this revision.Sep 7 2023, 8:04 AM

One question according to design:
Shouldn't the tooltip be aligned with the message?

image.png (468×1 px, 118 KB)

cc. @ted

(Sorry if this was discussed or if I am missing something, just sharing my thoughts from a "user perspective" as this tooltip placement is a bit confusing)

In D9097#268177, @kamil wrote:

One question according to design:
Shouldn't the tooltip be aligned with the message?

image.png (468×1 px, 118 KB)

cc. @ted

(Sorry if this was discussed or if I am missing something, just sharing my thoughts from a "user perspective" as this tooltip placement is a bit confusing)

Hey no worries, this is a good shout. The reason that the tooltip is positioned like that is because the tooltip has 3 parts the label, the action buttons, and the timestamp and we center align based on all three of those elements

Screenshot 2023-09-08 at 9.23.05 AM.png (344×1 px, 56 KB)

I can see that the placement is still bit wonky when the label is missing, but I feel like this might be outside the scope of this diff. I created a follow up linear task to eventually address this

https://linear.app/comm/issue/ENG-4879/make-the-message-action-tooltip-more-aligned-with-the-message

Hey no worries, this is a good shout. The reason that the tooltip is positioned like that is because the tooltip has 3 parts the label, the action buttons, and the timestamp and we center align based on all three of those elements

Screenshot 2023-09-08 at 9.23.05 AM.png (344×1 px, 56 KB)

I can see that the placement is still bit wonky when the label is missing, but I feel like this might be outside the scope of this diff. I created a follow up linear task to eventually address this

https://linear.app/comm/issue/ENG-4879/make-the-message-action-tooltip-more-aligned-with-the-message

thanks for your response @ginsu

This revision is now accepted and ready to land.Sep 10 2023, 11:30 PM
This revision was landed with ongoing or failed builds.Sep 11 2023, 6:01 AM
This revision was automatically updated to reflect the committed changes.