Page MenuHomePhabricator
Differential D13697

[native] Fix NUX Android problem
ClosedPublic
Actions

Authored by inka on Fri, Oct 11, 2:50 AM.
Tags
None
Referenced Files
F2984615: D13697.id45093.diff
Wed, Oct 16, 12:37 PM
F2983720: D13697.id45138.diff
Wed, Oct 16, 10:27 AM
Unknown Object (File)
Tue, Oct 15, 10:55 AM
Unknown Object (File)
Mon, Oct 14, 10:01 PM
Unknown Object (File)
Mon, Oct 14, 11:21 AM
Unknown Object (File)
Mon, Oct 14, 8:36 AM
Unknown Object (File)
Mon, Oct 14, 8:26 AM
Unknown Object (File)
Mon, Oct 14, 7:58 AM
View All 19 Files
Subscribers
ashoat

Details

Reviewers
tomek
kamil
Commits
rCOMMe43c6b922c25: [native] Fix NUX Android problem
Summary

issue: ENG-9472
On some Androids, we cannot measure a button that is not visible on the screen. undefineds are returned. So we have to wait of the login screen to be dismissed, before we call measure.
I wanted to pass the measure function itseft through conxtext, but that results in Cannot read property '_nativeTag' of undefined error. Not sure what that means, but I don't think passing the ref is much worse.
measure asynchronously measures the component and then calls the callback. This is by I need states and an effect, instead of a memo.

buttonMeasured flag ensures the tip doesn't jump after the measure is done. measure returns quickly and I don't think this slight delay is a problem.

Test Plan

Tested on Android 11 physical device, and iOS 17.4 simulator. Tesdted by updating NUXHandlerInner to run the effect on every app reaload.
Checked that on both Android and iOS the NUX tips are displayed in the correct position. Checked thare are no odd visual effects, like jumping (thanks to buttonMeasured)

Diff Detail

Repository
rCOMM Comm
Branch
inka/query_identity
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

inka created this revision.Fri, Oct 11, 2:50 AM
Herald added a subscriber: ashoat. · View Herald TranscriptFri, Oct 11, 2:50 AM
inka edited the summary of this revision. (Show Details)Fri, Oct 11, 2:51 AM
Harbormaster completed remote builds in B32196: Diff 45092.Fri, Oct 11, 3:11 AM
inka requested review of this revision.Fri, Oct 11, 3:11 AM
tomek requested changes to this revision.Fri, Oct 11, 4:53 AM
tomek added inline comments.
native/chat/chat-tab-bar.react.js
33–44

This pattern doesn't make too much sense to me - there shouldn't be any need to save the ref in the context on each layout - doing it once should be enough. Also, we don't need to useRef to handle that - we could use a callback ref https://pl.legacy.reactjs.org/docs/refs-and-the-dom.html#callback-refs and set the value in the context every time it is called.

native/tooltip/nux-tips-overlay.react.js
127–137

The types should be readonly

139–140

Why do we need to introduce a new state for this? Can't we make the states where we keep the measurement optional?

150–152

Is there a reason to keep these separated?

This revision now requires changes to proceed.Fri, Oct 11, 4:53 AM
tomek added a comment.Fri, Oct 11, 4:54 AM

I wanted to pass the measure function itseft through conxtext, but that results in Cannot read property '_nativeTag' of undefined error.

My guess is that we need to bind these methods.

inka updated this revision to Diff 45137.Mon, Oct 14, 1:57 AM

Address review. Going to try biding the methods now

Harbormaster completed remote builds in B32220: Diff 45137.Mon, Oct 14, 2:15 AM
inka updated this revision to Diff 45138.Mon, Oct 14, 2:19 AM

Bind measure

native/tooltip/nux-tips-overlay.react.js
139–140

I didn't like all the ifs this made me add to the code, but I can do it if you prefer that

Harbormaster completed remote builds in B32221: Diff 45138.Mon, Oct 14, 2:37 AM
tomek accepted this revision.Mon, Oct 14, 3:48 AM
tomek added inline comments.
native/chat/chat-tab-bar.react.js
36 ↗(On Diff #45138)

You can even define this outside the component.

native/components/nux-tips-context.react.js
4 ↗(On Diff #45138)

Are you sure this is the correct place to import from?

native/tooltip/nux-tips-overlay.react.js
139–140

Agree, this isn't ideal. However, I think it is better to have explicit empty objects and nulls before the measurement is concluded than to use the incorrect default values.

This revision is now accepted and ready to land.Mon, Oct 14, 3:48 AM
inka updated this revision to Diff 45147.Mon, Oct 14, 4:46 AM

Move callbacks out of components

native/components/nux-tips-context.react.js
4 ↗(On Diff #45138)

This is the only place where this flow type is defined in the react-native repo (I searched it on github). My IDE doesn't recognise any other possibilities either.

Harbormaster completed remote builds in B32225: Diff 45147.Mon, Oct 14, 5:06 AM
Closed by commit rCOMMe43c6b922c25: [native] Fix NUX Android problem (authored by inka). · Explain WhyMon, Oct 14, 7:24 AM
This revision was automatically updated to reflect the committed changes.
inka added a commit: rCOMMe43c6b922c25: [native] Fix NUX Android problem.

Revision Contents
Changeset List

PathSize
native/
chat/
16 lines
14 lines
components/
10 lines
tooltip/
45 lines

Diff 45092

View Options

native/chat/chat-tab-bar.react.js

Loading...
View Options

native/chat/chat.react.js

Loading...
View Options

native/components/nux-tips-context.react.js

Loading...
View Options

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

Loading...