Page MenuHomePhabricator
Differential D14108

[native] Move Reanimated nodes in Tooltip to functional component
ClosedPublic
Actions

Authored by angelika on Wed, Dec 11, 7:42 AM.
Tags
None
Referenced Files
F3507320: D14108.id46326.diff
Fri, Dec 20, 8:12 PM
F3507265: D14108.diff
Fri, Dec 20, 8:03 PM
F3507120: D14108.id46406.diff
Fri, Dec 20, 7:31 PM
F3506227: D14108.id46468.diff
Fri, Dec 20, 3:48 PM
F3506008: D14108.id46504.diff
Fri, Dec 20, 3:22 PM
F3504873: D14108.id46327.diff
Fri, Dec 20, 10:57 AM
F3498950: D14108.id46405.diff
Thu, Dec 19, 9:57 PM
Unknown Object (File)
Thu, Dec 19, 11:13 AM
View All 12 Files
Subscribers
None

Details

Reviewers
ashoat
tomek
Commits
rCOMMbc4723f1d644: [native] Move Reanimated nodes in Tooltip to functional component
Summary

Simply move reanimated nodes to functional component as a part of Tooltip migration to functional component.

Depends on D14107

Test Plan

Open up some tooltips and verify they're animated correctly

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

angelika created this revision.Wed, Dec 11, 7:42 AM
angelika held this revision as a draft.
angelika added a child revision: D14109: [native] Move styles in Tooltip to functional component.Wed, Dec 11, 7:45 AM
Harbormaster completed remote builds in B33008: Diff 46326.Wed, Dec 11, 8:48 AM
angelika published this revision for review.Wed, Dec 11, 4:54 PM
ashoat accepted this revision.Wed, Dec 11, 5:52 PM
ashoat added inline comments.
native/tooltip/tooltip.react.js
575 ↗(On Diff #46326)

useValue would be cleaner here

I guess this will get deleted later in the stack anyways... if so feel free to ignore the feedback

592–595 ↗(On Diff #46326)

One slight change here: now when the screen dimensions change, we will replace the Reanimated value. Generally this is an anti-pattern, but it probably doesn't matter here because (1) we only allow screen rotation in rare places in the app, and I don't think we have Tooltips there, (2) this probably gets replaced with worklets later anyways... not sure if the feedback will still be relevant after that

This revision is now accepted and ready to land.Wed, Dec 11, 5:52 PM
angelika updated this revision to Diff 46405.Fri, Dec 13, 5:31 AM

Rebase and address feedback

angelika marked 2 inline comments as done.Fri, Dec 13, 6:12 AM
angelika added inline comments.
native/tooltip/tooltip.react.js
575 ↗(On Diff #46326)

It's replaced with useSharedValue later in the stack. In this stack I wanted to keep reanimated v1 api and make a migration in a separate diff.

592–595 ↗(On Diff #46326)

It's put in a worklet in a later diff.

Harbormaster completed remote builds in B33083: Diff 46405.Fri, Dec 13, 6:33 AM
angelika updated this revision to Diff 46468.Mon, Dec 16, 6:20 AM
angelika marked 2 inline comments as done.

Rebase

Harbormaster completed remote builds in B33127: Diff 46468.Mon, Dec 16, 7:27 AM
Closed by commit rCOMMbc4723f1d644: [native] Move Reanimated nodes in Tooltip to functional component (authored by angelika). · Explain WhyMon, Dec 16, 1:34 PM
This revision was automatically updated to reflect the committed changes.
angelika added a commit: rCOMMbc4723f1d644: [native] Move Reanimated nodes in Tooltip to functional component.

Revision Contents
Changeset List

PathSize
native/
tooltip/
172 lines

Diff 46468

View Options

native/tooltip/tooltip.react.js

Loading...