Page MenuHomePhabricator

[native] Add nux context
ClosedPublic

Authored by inka on Aug 5 2024, 1:38 AM.
Tags
None
Referenced Files
F3203666: D12963.diff
Sat, Nov 9, 8:10 PM
F3193575: D12963.id43341.diff
Fri, Nov 8, 11:05 PM
Unknown Object (File)
Fri, Nov 8, 12:24 PM
Unknown Object (File)
Fri, Nov 8, 12:24 PM
Unknown Object (File)
Wed, Nov 6, 11:33 PM
Unknown Object (File)
Sun, Nov 3, 10:07 PM
Unknown Object (File)
Sun, Nov 3, 10:07 PM
Unknown Object (File)
Sun, Nov 3, 10:07 PM
Subscribers

Details

Summary

issue: ENG-8618
To be able to get coordinest of buttons, we need to let them render first. Then, they will register their coordinates in this context. A handler component will select those coordinates, and navigate to screens passing the coordinates in params

Test Plan

Tested with following diffs. Testeds that after a button registares their coordinates the handler effect is fired and nux tip appears on the screen, rendering the button in the cprrect place

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Aug 5 2024, 1:53 AM
Harbormaster failed remote builds in B30881: Diff 43061!
inka requested review of this revision.Aug 5 2024, 7:29 AM

Just a couple nits

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

How about nuxTip? It seems like a really short/generic variable name

10 ↗(On Diff #43100)

This could be NUXTip

tomek requested changes to this revision.Aug 6 2024, 1:54 AM
tomek added inline comments.
native/components/nux-tips-context.react.js
12–21 ↗(On Diff #43100)

What's the purpose of it? If we want to have one type with optional and one with mandatory fields, we can create a type with app the fields required and use https://flow.org/en/docs/types/utilities/#toc-partial for optional.

21 ↗(On Diff #43100)

This gets deprecated in flow 0.211 but we're not there yet. An alternative https://flow.org/en/docs/types/mapped-types/ is supported from 0.210.

38–54 ↗(On Diff #43100)

It seems like the intention here was to avoid a render after calling registerTipButton - which is good. But there's a small issue with the memo - it is fragile because we're mutating its content, which breaks intuitions about how memos work. Instead, we should use a ref to store the tipsProps.

58–61 ↗(On Diff #43100)

This is hacky. Can we store an empty object in tipsProps initially, and then fill it when calling registerTipButton? Here we would need to check the presence of keys from tip values.

This revision now requires changes to proceed.Aug 6 2024, 1:54 AM

Address review. RequiredTipProps were dropped altogether after applying other review suggestions

tomek added inline comments.
native/components/nux-tips-context.react.js
25 ↗(On Diff #43261)

We're returning the value from tipsProps ref directly, so we need to make sure that the caller won't mutate it.

42–45 ↗(On Diff #43261)

Is there a downside to simply overriding the whole object?

This revision is now accepted and ready to land.Aug 8 2024, 7:54 AM

Make it possible to unregister

This revision was automatically updated to reflect the committed changes.