Page MenuHomePhabricator

[native] Add <next> button to nux tips
ClosedPublic

Authored by inka on Aug 14 2024, 5:41 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Oct 24, 1:39 AM
Unknown Object (File)
Fri, Oct 18, 9:48 PM
Unknown Object (File)
Fri, Oct 18, 9:48 PM
Unknown Object (File)
Fri, Oct 18, 9:48 PM
Unknown Object (File)
Fri, Oct 18, 9:48 PM
Unknown Object (File)
Fri, Oct 18, 9:48 PM
Unknown Object (File)
Fri, Oct 18, 9:42 PM
Unknown Object (File)
Fri, Oct 18, 9:38 PM
Subscribers

Details

Summary

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

image.png (2×1 px, 285 KB)

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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

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

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

Dismiss one tip before navigating to the next

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
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

Have to answer to the rest of the review

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

  1. Can we add some margin around the tooltip?
  2. Can we reduce the font size somewhat?
  3. Can we reduce the bottom padding below the button?
  4. 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

This revision is now accepted and ready to land.Aug 20 2024, 4:32 AM