Page MenuHomePhabricator

[web] Modify `findTooltipPosition` to support new tooltip positioning logic
ClosedPublic

Authored by jacek on Aug 23 2022, 4:55 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 25, 10:00 AM
Unknown Object (File)
Mon, Nov 25, 9:36 AM
Unknown Object (File)
Mon, Nov 25, 6:55 AM
Unknown Object (File)
Mon, Nov 11, 1:52 AM
Unknown Object (File)
Oct 28 2024, 5:06 AM
Unknown Object (File)
Oct 28 2024, 5:06 AM
Unknown Object (File)
Oct 28 2024, 5:06 AM
Unknown Object (File)
Oct 28 2024, 5:06 AM
Subscribers

Details

Summary

Refactor findTooltipPosition by removing size calculation from this function and adding new TOP and BOTTOM positions. Also made the return value optional, as we want to handle the case where it is impossible to display the tooltip anywhere.

Now, the naming convention for position is the following:

  • the first word indicates on which border of the source object we want to display the tooltip.
  • the second word indicates the direction we want to align the tooltip

So LEFT_BOTTOM means we display the tooltip on the left of the source, and it is aligned to the bottom of the source - for examples see: https://linear.app/comm/issue/ENG-1575#comment-ef9727c3

Test Plan

Flow; Jest tests; Tested with the rest of diffs stack.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

jacek held this revision as a draft.
web/chat/tooltip.react.js
36 ↗(On Diff #15854)

Change to temporarily avoid flow issues. Will be changed in next diffs

tomek requested changes to this revision.Aug 26 2022, 6:18 AM

This code is quite complicated, but this is understandable... could you write some tests for it?

web/chat/composed-message.react.js
29 ↗(On Diff #15854)

It might be a good idea to explain the reasoning behind this rename. Also, the rename should've been a separate diff.

web/chat/tooltip-utils.js
40 ↗(On Diff #15854)

How do we determine this value? Is there a way to compute it instead of hardcoding?

59 ↗(On Diff #15854)

I'm afraid of having an option to not display a tooltip at all. It might be possible that for a long message all the predefined positions are outside the viewport but still there's enough place for a tooltip. At least we should have a followup task for this (if it's hard to find a proper solution now).

BTW, is there a downside in using the previous approach of returning the last available position when none are appropriate?

This revision now requires changes to proceed.Aug 26 2022, 6:18 AM
web/chat/tooltip-utils.js
40 ↗(On Diff #15854)

It is hardcoded in style.css (64px height + 1px for border):

div.main-header {
  height: 64px;
  background: var(--bg);
  display: flex;
  align-items: center;
  border-bottom: 1px solid var(--border-color);
}

Other styles calculating layout also have it hardcoded:

div.layout {
  height: 100vh;
  display: grid;
  grid-template-columns: 244px repeat(12, 1fr);
  grid-template-rows: 65px calc(100vh - 65px);
  grid-template-areas:
    'nav nav nav nav nav nav nav nav nav nav nav nav nav'
    'sBar app app app app app app app app app app app app';
}

So I'm not sure if there is an easy way to compute this value now, as it's already harcoded in multiple places

59 ↗(On Diff #15854)

BTW, is there a downside in using the previous approach of returning the last available position when none are appropriate?

I can leave an old solution here - using the last available position

web/chat/tooltip-utils.js
59 ↗(On Diff #15854)

How should we handle the case if an empty array of availablePositions is provided? In such case, an old solution would return undefined although its result is not optional, as the flow doesn't check an existence of array element, isn't it?

web/chat/tooltip-utils.js
59 ↗(On Diff #15854)

Discussed this with Tomek. We will add a defaultPosition parameter to this function.

Change return type to non-optional & add tests

The file should be probably moved into web/utils (as the tests are there so it could be run), but I'll cover this with separate diff.

Haven't verified the logic too closely, but overall the code looks ok and tests should be able to validate the solution.

web/chat/tooltip-utils.js
40 ↗(On Diff #15854)

Ok, let's keep it but we should be mindful when introducing the new components that hardcoding like this will greatly reduce the maintainability. Using styled components is a great solution for that issue.

web/utils/tooltip-utils.test.js
80–81 ↗(On Diff #16055)

We probably should set back the previous values after the tests are done. We can do that at the end of describe or by using beforeAll and afterAll functions.

This revision is now accepted and ready to land.Aug 30 2022, 8:55 AM

add afterAll to set default window width and height in tests