Page MenuHomePhabricator

[web] Show message tooltip on hover in the pinned messages modal
ClosedPublic

Authored by rohan on Apr 21 2023, 6:49 AM.
Tags
None
Referenced Files
F3392245: D7563.id25531.diff
Sat, Nov 30, 7:46 AM
Unknown Object (File)
Thu, Nov 28, 10:21 AM
Unknown Object (File)
Wed, Nov 27, 10:51 PM
Unknown Object (File)
Wed, Nov 27, 10:45 PM
Unknown Object (File)
Wed, Nov 27, 10:30 PM
Unknown Object (File)
Wed, Nov 27, 8:48 PM
Unknown Object (File)
Thu, Nov 21, 3:22 PM
Unknown Object (File)
Sat, Nov 16, 2:15 AM
Subscribers

Details

Summary

Currently, the tooltip displays below the pinned messages modal when a message is hovered over. This is because of the z-index property in the ModalOverlay component.

The solution here is to make the tooltip container, icons, and label to have a z-index of 5 so it displays above the modal. This visually has no effect on the tooltip in chat.

Linear: https://linear.app/comm/issue/ENG-3451/show-message-tooltip-on-hover-in-the-pinned-messages-modal

Depends on D7505

Test Plan

Please watch the video showing the behavior of the tooltip in both chat and the pinned messages modal

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

web/utils/tooltip-action-utils.js
47 ↗(On Diff #25531)

This is the same approach as in https://phab.comm.dev/D7382

rohan requested review of this revision.Apr 21 2023, 7:07 AM
tomek requested changes to this revision.Apr 25 2023, 3:06 AM

Is there a way to avoid specifying z-index? (usually when we use z-index in one place, it quickly becomes necessary in a lot of places) If there's no other way, can we limit the number of places where it has to be specified?

This revision now requires changes to proceed.Apr 25 2023, 3:06 AM

Limit instances of z-index

In D7563#225479, @tomek wrote:

Is there a way to avoid specifying z-index? (usually when we use z-index in one place, it quickly becomes necessary in a lot of places) If there's no other way, can we limit the number of places where it has to be specified?

I believe the z-index is necessary since we're rendering the message in the ModalOverlay, and that has a z-index of 4, so by default I think the tooltip will render below the modal.

Screenshot 2023-04-25 at 6.29.07 AM.png (1×3 px, 552 KB)

I think alternatively, though like you say, we can limit the number of 'z-indexes' to just messageTooltipContainer and emojiKeyboard, and add a position: relative to messageTooltipContainer since z-index only works on positioned elements.

rohan added inline comments.
web/utils/tooltip-action-utils.js
61 ↗(On Diff #25649)

This is the same approach as in https://phab.comm.dev/D7382

I believe the z-index is necessary since we're rendering the message in the ModalOverlay, and that has a z-index of 4, so by default I think the tooltip will render below the modal.

Ahhh, that's exactly what I've described. The fact that z-index is used in one place made it necessary to use it in the other.

I think alternatively, though like you say, we can limit the number of 'z-indexes' to just messageTooltipContainer and emojiKeyboard, and add a position: relative to messageTooltipContainer since z-index only works on positioned elements.

That's a good idea!

This revision is now accepted and ready to land.Apr 25 2023, 3:42 PM