issue: ENG-8537
Details
Details
- Reviewers
tomek will ashoat - Commits
- rCOMM8cb6e94d2b05: [native] Add nux handler
Tested that when a user opens the app of the first time and logs in, the tips are displayed
Tested that a subsequent login doesn't result in tips being shown
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/components/nux-handler.react.js | ||
|---|---|---|
| 20–22 ↗ | (On Diff #43392) | I probably need to come up with a better condition, because this should probably only show up after registration, not after every login... |
| native/components/nux-handler.react.js | ||
|---|---|---|
| 20–22 ↗ | (On Diff #43392) | Ginsu recently introduced alertStore, which is meant to help with this sort of thing. Not sure if you're aware or using it already, but it lets us persist some state to say that an alert has already been shown, to avoid doing it multiple times |
| native/components/nux-handler.react.js | ||
|---|---|---|
| 30–34 ↗ | (On Diff #43392) | Can we create these params based on the specification of the tips (somewhere deeper in the stack where we have a mapping between a tip and a next tip)? |
| native/components/nux-tips-context.react.js | ||
| 69 ↗ | (On Diff #43392) | We shouldn't mutate the content of the memo. If we need to do something like this, we should use a state and set a new value based on the old one. |
| native/components/nux-handler.react.js | ||
|---|---|---|
| 20–22 ↗ | (On Diff #43392) | alertStore gets reset on logout, so it doesn't achieve what we want |
| native/components/nux-tips-context.react.js | ||
|---|---|---|
| 30 ↗ | (On Diff #43503) | |
| 31–35 ↗ | (On Diff #43503) | Interesting approach, and gets the job done! It might be quite confusing because firstTip isn't a tip and it doesn't make sense for it to have a tooltipLocation, but I don't think we have to spend too much time searching for a better solution. |
| 77 ↗ | (On Diff #43503) | |
| 91 ↗ | (On Diff #43503) | I think it would be even better if it was a memo instead. |
| 92–100 ↗ | (On Diff #43503) | I don't think we have to create a copy of the value. You can also consider using values(nuxTip).every(type => tipsProps[type]). |
Comment Actions
Requesting changes because it seems like alertStore can be used here, and I'd rather avoid reimplementing logic when we can share it instead
| native/components/nux-handler.react.js | ||
|---|---|---|
| 20–22 ↗ | (On Diff #43392) | Could we use a Redux migration to populate alertStore in such a way that the NUX won't get shown for clients that were logged-in at the time of the migration? |
Comment Actions
Open to building a separate mechanism here if it's necessary, or if the use case doesn't match alertStore very well, or if minimal code will be replaced because the existing approach is so simple. Curious for @tomek's take here as well
Comment Actions
Open to building a separate mechanism here if it's necessary
Is using useOnFirstLaunchEffect bad somehow? Or do we care about the tips not showing up for users who are already logged in?
| native/components/nux-handler.react.js | ||
|---|---|---|
| 20–22 ↗ | (On Diff #43392) | That should not be a problem. |
Comment Actions
I'm also really confused by this comment. We're using an existing mechanism and we're not reimplementing the logic. Also, it seems like useOnFirstLaunchEffect better expresses the requirements.
| native/components/nux-tips-context.react.js | ||
|---|---|---|
| 27–46 ↗ | (On Diff #43503) | I was thinking about it and it seems we can avoid introducing an artificial entry by storing the first tip as a separate variable. Would that work? |
Comment Actions
I don't love that we have two separate mechanisms for this. alertStore's purpose is to record when we show various alerts, and help us control how often we show them based on when they're been shown before. That's basically this use case as well. If there are some differences, I would think it would be better to extend alertStore to do what we want here (eg. use useOnFirstLaunchEffect or persist in a similar fashion) rather than building a separate mechanism.
I'm going to resign as I'm heading to vacation starting tomorrow and it doesn't appear that we have the time to do things right here.
| native/components/nux-handler.react.js | ||
|---|---|---|
| 49 ↗ | (On Diff #43503) | Will useOnFirstLaunchEffect re-run the NUX if the user logs out and back in as another account? (Not sure what we desire here, but I'd like to understand it better) |
Comment Actions
Ahhh, ok. So the NUX tips are kind of alerts, and we think that they should the same mechanism the alerts use. That makes sense to me, but the approach from this diff is pragmatic - we're using an existing utility that doesn't introduce any new logic. I don't think this code will change frequently, and if we ever decide to modify the logic, we could then modify it to use the alert store.
rather than building a separate mechanism.
This still confuses me. We're using an existing mechanism, that is really generic and we're not modifying it in any way. Not sure what is the new thing we're building here.
| native/components/nux-handler.react.js | ||
|---|---|---|
| 49 ↗ | (On Diff #43503) | This will run only once. Only uninstalling the application will allow rerunning the function.
I believe that it is unlikely that someone new will log in to the app where another person has logged out. So we can assume, that usually when one person sees the NUX, we don't need to show them it again after logging in again (even as another account because it is likely the same person). |