Page MenuHomePhabricator
Differential D14111

[native] Move methods from Tooltip to functional compononent
ClosedPublic
Actions

Authored by angelika on Wed, Dec 11, 7:54 AM.
Tags
None
Referenced Files
F3509992: D14111.id46507.diff
Sat, Dec 21, 10:06 AM
F3509313: D14111.id46408.diff
Sat, Dec 21, 3:22 AM
F3508435: D14111.id46329.diff
Fri, Dec 20, 10:05 PM
Unknown Object (File)
Fri, Dec 20, 12:57 PM
Unknown Object (File)
Fri, Dec 20, 11:48 AM
Unknown Object (File)
Thu, Dec 19, 10:06 PM
Unknown Object (File)
Thu, Dec 19, 4:18 PM
Unknown Object (File)
Thu, Dec 19, 12:55 PM
View All 15 Files
Subscribers
None

Details

Reviewers
ashoat
tomek
Commits
rCOMM613958367d7f: [native] Move methods from Tooltip to functional compononent
Summary

Move getTooltipItem, onPressMore, renderMoreIcon and onTooltipContainerLayout to the functional compoent as a part of Tooltip migration to functional component.

Depends on D14110

Test Plan

Open up some tooltips and verify they're looking correctly.
Open text message tooltip and press More, verify it works.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

angelika created this revision.Wed, Dec 11, 7:54 AM
angelika held this revision as a draft.
angelika edited the summary of this revision. (Show Details)Wed, Dec 11, 7:54 AM
angelika edited the test plan for this revision. (Show Details)
angelika added a child revision: D14112: [native] Move render function from Tooltip to functional component.Wed, Dec 11, 7:59 AM
Harbormaster completed remote builds in B33011: Diff 46329.Wed, Dec 11, 9:18 AM
angelika published this revision for review.Wed, Dec 11, 4:55 PM
ashoat accepted this revision.Wed, Dec 11, 5:55 PM
ashoat added inline comments.
native/tooltip/tooltip.react.js
562–566 ↗(On Diff #46329)

We don't need to convert this into a callback... and in this case, I don't think we need a React.useMemo either. Arguably we can just pass in tooltipItem={boundTooltipItem}

573–578 ↗(On Diff #46329)

Can be a memo

This revision is now accepted and ready to land.Wed, Dec 11, 5:55 PM
angelika updated this revision to Diff 46408.Fri, Dec 13, 5:33 AM
angelika edited the test plan for this revision. (Show Details)

Rebase and address feedback

angelika marked an inline comment as done.Fri, Dec 13, 6:16 AM
angelika added inline comments.
native/tooltip/tooltip.react.js
573–578 ↗(On Diff #46329)

I can't put renderMoreIcon in a memo, because BaseTooltipItem expects a callback, not a component

ashoat added inline comments.Fri, Dec 13, 6:17 AM
native/tooltip/tooltip.react.js
573–578 ↗(On Diff #46329)

Why can't you change BaseTooltipItem to take a React.Node though?

Harbormaster completed remote builds in B33086: Diff 46408.Fri, Dec 13, 6:59 AM
ashoat added inline comments.Fri, Dec 13, 6:35 PM
native/tooltip/tooltip.react.js
573–578 ↗(On Diff #46329)

I didn't realize it's a whole different file... sorry about this, please ignore my feedback here :)

angelika updated this revision to Diff 46471.Mon, Dec 16, 6:21 AM

Rebase

Harbormaster completed remote builds in B33130: Diff 46471.Mon, Dec 16, 7:53 AM
Closed by commit rCOMM613958367d7f: [native] Move methods from Tooltip to functional compononent (authored by angelika). · Explain WhyMon, Dec 16, 1:34 PM
This revision was automatically updated to reflect the committed changes.
angelika added a commit: rCOMM613958367d7f: [native] Move methods from Tooltip to functional compononent.

Revision Contents
Changeset List

PathSize
native/
tooltip/
98 lines

Diff 46507

View Options

native/tooltip/tooltip.react.js

Loading...