Page MenuHomePhabricator
Differential D12980

[native] Move onTipContainerLayout
ClosedPublic
Actions

Authored by inka on Aug 5 2024, 6:05 AM.
Tags
None
Referenced Files
F3203849: D12980.diff
Sat, Nov 9, 8:29 PM
F3193578: D12980.id43344.diff
Fri, Nov 8, 11:06 PM
Unknown Object (File)
Fri, Nov 8, 12:24 PM
Unknown Object (File)
Wed, Nov 6, 11:17 PM
Unknown Object (File)
Sun, Nov 3, 10:10 PM
Unknown Object (File)
Sun, Nov 3, 10:10 PM
Unknown Object (File)
Sun, Nov 3, 9:54 PM
Unknown Object (File)
Fri, Oct 18, 9:47 PM
View All 61 Files
Subscribers
ashoat

Details

Reviewers
tomek
will
Commits
rCOMMb0ad2ef37bb1: [native] Move onTipContainerLayout
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 created this revision.Aug 5 2024, 6:05 AM
Herald added a subscriber: ashoat. · View Herald TranscriptAug 5 2024, 6:05 AM
inka added a parent revision: D12979: [native] Move buttonStyle.Aug 5 2024, 6:12 AM
inka added a child revision: D12981: [native] Move tipContainerOpacity, tipVerticalBelow, tipHeight, margin.
Harbormaster completed remote builds in B30927: Diff 43115.Aug 5 2024, 8:21 AM
inka requested review of this revision.Aug 5 2024, 8:21 AM
ashoat added inline comments.Aug 5 2024, 11:37 AM
native/tooltip/nux-tips-overlay.react.js
300

It's a bit hacky to use a useMemo for this. I think the usual "best practice" is that an instance variable from a class component becomes a useRef in a function component. This is functionally the same in our configuration of React, but it might become more important in "concurrent mode", if we ever enable that (might be related to React Native "new architecture")

tomek accepted this revision.Aug 6 2024, 2:25 AM
tomek added inline comments.
native/tooltip/nux-tips-overlay.react.js
300

Another reason for using ref here is that we usually assume that values returned by memos are immutable. The value that we have here can mutate without its reference being changed, which is confusing.

319

We should make the dependencies more specific.

This revision is now accepted and ready to land.Aug 6 2024, 2:25 AM
inka updated this revision to Diff 43344.Aug 13 2024, 6:48 AM

Address review

Harbormaster completed remote builds in B31066: Diff 43344.Aug 13 2024, 7:05 AM
Closed by commit rCOMMb0ad2ef37bb1: [native] Move onTipContainerLayout (authored by inka). · Explain WhyAug 13 2024, 7:46 AM
This revision was automatically updated to reflect the committed changes.
inka added a commit: rCOMMb0ad2ef37bb1: [native] Move onTipContainerLayout.

Revision Contents
Changeset List

PathSize
native/
tooltip/
53 lines

Diff 43115

View Options

native/tooltip/nux-tips-overlay.react.js

Loading...