Page MenuHomePhabricator

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

Authored by inka on Aug 5 2024, 6:06 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Sep 18, 11:46 AM
Unknown Object (File)
Sat, Sep 14, 8:44 PM
Unknown Object (File)
Mon, Sep 9, 6:32 PM
Unknown Object (File)
Mon, Sep 9, 7:44 AM
Unknown Object (File)
Fri, Sep 6, 10:32 AM
Unknown Object (File)
Thu, Sep 5, 3:52 PM
Unknown Object (File)
Thu, Sep 5, 3:52 PM
Unknown Object (File)
Thu, Sep 5, 3:51 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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inka requested review of this revision.Aug 5 2024, 8:36 AM
native/tooltip/nux-tips-overlay.react.js
115–119 ↗(On Diff #43117)

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 ↗(On Diff #43117)

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 ↗(On Diff #43117)

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 ↗(On Diff #43117)

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 ↗(On Diff #43117)

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