Page MenuHomePhabricator

[web] introduce calculateNavigationSidebarTooltipSize
ClosedPublic

Authored by ginsu on Dec 15 2023, 12:36 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 20 2024, 7:16 AM
Unknown Object (File)
Feb 20 2024, 7:08 AM
Unknown Object (File)
Feb 20 2024, 6:02 AM
Unknown Object (File)
Feb 20 2024, 3:16 AM
Unknown Object (File)
Feb 19 2024, 6:43 PM
Unknown Object (File)
Feb 14 2024, 5:07 AM
Unknown Object (File)
Jan 8 2024, 6:22 PM
Unknown Object (File)
Jan 6 2024, 3:00 PM
Subscribers

Details

Summary

Part of the process when we create tooltips is that we need to calculate the size of the tooltip. This diff introduces the function that will handle calculating the size of the tooltip.

Part of https://linear.app/comm/issue/ENG-5948/hovering-over-every-clickable-element-in-this-sidebar-will-have-a

Test Plan

Logged out results of this function also inspected the tooltip element (w/o passing the style prop into the tooltip) and confirmed that I was getting the correct size

Screenshot 2023-12-15 at 3.38.56 PM.png (1×3 px, 1 MB)

Screenshot 2023-12-15 at 3.38.44 PM.png (1×3 px, 1 MB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: atul, rohan, kamil.
ginsu requested review of this revision.Dec 15 2023, 1:07 PM
atul added reviewers: ashoat, tomek.

At a high level this seems correct to me, but I'm not super familiar with all of the tooltip math.

On the surface, it doesn't seem great that we have hard coded constants for eg marginLeft defined within this function?

Adding @ashoat/@tomek who can probably give this a better review.

This revision is now accepted and ready to land.Dec 20 2023, 9:51 PM
web/utils/tooltip-utils.js
422–425 ↗(On Diff #34744)

For me, it doesn't sound right to include a margin within a size. A margin tells how far a thing is shifted and doesn't make it any bigger. Can we somehow avoid including it? Why do we include it in the calculation?

web/utils/tooltip-utils.js
422–425 ↗(On Diff #34744)

The purpose of this function is to calculate the size of the navigation tooltip component so that we can use this to determine where is the best place to render the tooltip. Based on how the tooltip component is built, the blue highlighted area is considered the tooltip component (which includes the margin):

Screenshot 2023-12-15 at 3.38.56 PM.png (1×3 px, 1 MB)

If we don't consider the margin in this calculation the width of the tooltip component will be 24px short and could lead to the tooltip being rendered to the right and overflowing by 24px instead of being rendered to the left or another direction of its origin point (not likely in this case since this tooltip is on the far left of the screen, but hopefully this example explains my point)

I definitely should factor out the margin size, but let me know if you guys have any more questions about the margin!

address comments + rebase before landing

forgot to hit save on one file