Details

Reviewers

tomek
will

Commits

rCOMM30047630da0e: [native] Add <next> button to nux tips

Summary

issue: ENG-8617
This button will mavigete to the next tip and dismiss the tips in the last tip.

Test Plan

Tested with following diffs, tested that the button navigates to the next tip and dismisses on the last tip

Diff Detail

Repository

rCOMM Comm

Branch

inka/nux

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

inka created this revision.Aug 14 2024, 5:41 AM

Herald added a subscriber: ashoat. · View Herald TranscriptAug 14 2024, 5:41 AM

inka added a parent revision: D13079: [native] Fetch coordinates from context in nux tips overlay.Aug 14 2024, 5:41 AM

inka added a child revision: D13081: [native] Extract community drawer button for nux tips, create community drawer tip.Aug 14 2024, 5:44 AM

Harbormaster completed remote builds in B31095: Diff 43381.Aug 14 2024, 5:57 AM

inka requested review of this revision.Aug 14 2024, 5:57 AM

inka edited the summary of this revision. (Show Details)Aug 14 2024, 6:20 AM

Some visual requests, feel free to do this in a separate task / diff if you prefer:

Can we add some margin around the tooltip?
Can we reduce the font size somewhat?
Can we reduce the bottom padding below the button?
Can we make the button a little smaller?

Dismiss one tip before navigating to the next

Harbormaster failed remote builds in B31103: Diff 43389!Aug 14 2024, 7:15 AM

tomek requested changes to this revision.Aug 14 2024, 9:30 AM

tomek added inline comments.

native/tooltip/nux-tips-overlay.react.js
375–379 ↗	(On Diff #43389)	Wondering if going back and navigating to a new screen can cause some issues - e.g. could it render an app in a state where the tooltip isn't shown before we show the next tooltip? Have you checked if we can set the navigation params without actually navigating?
416 ↗	(On Diff #43389)	We shouldn't use `RegistrationButton` here, because it isn't a registration. Is there a reusable component we can use? Or maybe, we should rename this button to become more reusable?

This revision now requires changes to proceed.Aug 14 2024, 9:30 AM

tomek added inline comments.Aug 14 2024, 10:03 AM

native/tooltip/nux-tips-overlay.react.js
375–379 ↗	(On Diff #43389)	Another option could be to group all the tips inside one stack navigator and use `replace` instead of `navigate`. https://reactnavigation.org/docs/stack-navigator/#replace

inka mentioned this in D13118: [native] Rename RegistrationButton.Aug 20 2024, 12:33 AM

inka added a parent revision: D13118: [native] Rename RegistrationButton.Aug 20 2024, 12:34 AM

inka removed a parent revision: D13079: [native] Fetch coordinates from context in nux tips overlay.

Rename

Have to answer to the rest of the review

Harbormaster completed remote builds in B31157: Diff 43472.Aug 20 2024, 12:56 AM

In D13080#369411, @ashoat wrote:

Some visual requests, feel free to do this in a separate task / diff if you prefer:

Can we add some margin around the tooltip?

Can we reduce the font size somewhat?

Can we reduce the bottom padding below the button?

Can we make the button a little smaller?

Created ENG-9058 to address this, and the opacity blinking effect

native/tooltip/nux-tips-overlay.react.js
375–379 ↗	(On Diff #43389)	This should behave like this, because the tips have exiting and entering animations, so they should not replace one another instantly. We discussed this and agreed this should be left. But we should stop animating the background opacity between tips, to avoid blinking effect