Page MenuHomePhabricator

[native] Move tipContainerOpacity, tipVerticalBelow, tipHeight, margin
ClosedPublic

Authored by inka on Aug 5 2024, 6:06 AM.
Tags
None
Referenced Files
F3503615: D12981.id43117.diff
Fri, Dec 20, 5:54 AM
F3502222: D12981.id43430.diff
Fri, Dec 20, 3:59 AM
F3499591: D12981.diff
Thu, Dec 19, 11:35 PM
Unknown Object (File)
Nov 20 2024, 5:19 AM
Unknown Object (File)
Nov 9 2024, 11:38 PM
Unknown Object (File)
Nov 6 2024, 11:17 PM
Unknown Object (File)
Oct 18 2024, 9:47 PM
Unknown Object (File)
Oct 18 2024, 9:47 PM
Subscribers

Details

Summary

issue: ENG-8914
Refactoring from class to function

Test Plan

Tested by creating a nux tooltip and displaying it - still works

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

inka requested review of this revision.Aug 5 2024, 8:36 AM
native/tooltip/nux-tips-overlay.react.js
115–119

Given their similarities, curious that tipVerticalBelow was moved but tipVerticalAbove wasn't. I guess tipVerticalAbove will be moved in a later diff?

native/tooltip/nux-tips-overlay.react.js
115–119

Ah, looks like tipVerticalAbove was deleted in D12984

Any background on why we decided to preserve only one direction of the tips?

This revision is now accepted and ready to land.Aug 6 2024, 2:27 AM
native/tooltip/nux-tips-overlay.react.js
115–119

For all current use cases we only need an upward facing triangle. I also wanted to remove as much code as possible, to make this easier to refactor. I think once all of this code is refactored, adding the triangle down, if needed, will be easy. And it helps me to have as little logic as possible.

native/tooltip/nux-tips-overlay.react.js
115–119

I don't think it makes sense to remove the downwards triangle, given that it's definitely useful in a NUX context. I don't see the point of removing and then re-adding "if needed"

native/tooltip/nux-tips-overlay.react.js
115–119

I had already landed some of the code that was removing this logic before I saw this comment (most of the logic related to tooltipLocation was removed in D12942) , so I added a diff at the top of the stack adding this logic back: D13072