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

Authored by ginsu on Sep 7 2023, 7:46 AM.

Details

Reviewers

atul
kamil
michal

Commits

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

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

ginsu created this revision.Sep 7 2023, 7:46 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptSep 7 2023, 7:46 AM

ginsu edited the summary of this revision. (Show Details)Sep 7 2023, 7:54 AM

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

ginsu added reviewers: atul, kamil, michal.

ginsu attached a referenced file: F746261: Screen Recording 2023-09-07 at 10.49.08 AM.mov. (Show Details)

ginsu attached a referenced file: F746256: Screen Recording 2023-09-07 at 10.49.36 AM.mov. (Show Details)

ginsu added inline comments.Sep 7 2023, 8:02 AM

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

Harbormaster completed remote builds in B22425: Diff 30837.Sep 7 2023, 8:04 AM

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?

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?

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

ginsu attached a referenced file: F747696: Screenshot 2023-09-08 at 9.23.05 AM.png. (Show Details)Sep 8 2023, 6:37 AM

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

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

rebase before landing

This revision was landed with ongoing or failed builds.Sep 11 2023, 6:01 AM

Closed by commit rCOMM47bca6f563e2: [web] fix message action tooltip placement for messages with inline engagement (authored by ginsu). · Explain Why

This revision was automatically updated to reflect the committed changes.

ginsu added a commit: rCOMM47bca6f563e2: [web] fix message action tooltip placement for messages with inline engagement.

Harbormaster completed remote builds in B22486: Diff 30917.Sep 11 2023, 6:16 AM