Onboarding persistence #1486
Onboarding persistence #1486
Conversation
| // Checks if the hook has already been refreshed. | ||
| // Sometimes, it takes a moment for useNavigation to update from the previous route's values, | ||
| // so we need to perform this check to ensure we have updated values of previousPath | ||
| const hookUpdated = currentPath === pathname; |
There was a problem hiding this comment.
@ncarazon This one is kinda workaround, could you please tell me if it's ok?
There was a problem hiding this comment.
The workaround looks legit. However, I see another potential issue with the logic to skip the onboarding flow: the onboarding flow will still be triggered after skipping it if navigated from a route different from home or questions feed. For example: home -> tournaments -> questions. Is it expected? Ideally, we would utilize React Context, that would store a flag for skipping the onboarding. This could also simplify the code of OnboardingCheck, as we wouldn't need to update previousPathHasTutorial each time we want to add an onboarding trigger on a new page.
There was a problem hiding this comment.
Yeah, this is what @SylvainChevalier wanted: we should encourage users to complete onboarding if they have not, so I assume we do like to show onboarding again.
This specific fix is just about not showing onboarding on the next consequent page with onboarding component
There was a problem hiding this comment.
In that case, could we slightly refactor OnboardingCheck so this component would be the only place where onboarding triggers would be defined? Right now, if we want to add a new route to onboarding triggers, we would need to render the OnboardingCheck component on the page and additionally update the path array in previousPathHasTutorial. This may not be obvious for other engineers, so ideally, we would move OnboardingCheck into the root layout and trigger the onboarding modal based on the current path.
I don't think this is a blocker for merging though, so I'm approving. You can create a technical issue to do the refactor and assign it to me if you want. I'll handle it when there is time.
There was a problem hiding this comment.
@ncarazon It turned out skip button should permanently hide this tutorial, so no need to have this advanced previous page check. Thus, we don't need to hardcode list of pathes where this is used, so I assume we can keep this component as is without root layout
| // Checks if the hook has already been refreshed. | ||
| // Sometimes, it takes a moment for useNavigation to update from the previous route's values, | ||
| // so we need to perform this check to ensure we have updated values of previousPath | ||
| const hookUpdated = currentPath === pathname; |
There was a problem hiding this comment.
In that case, could we slightly refactor OnboardingCheck so this component would be the only place where onboarding triggers would be defined? Right now, if we want to add a new route to onboarding triggers, we would need to render the OnboardingCheck component on the page and additionally update the path array in previousPathHasTutorial. This may not be obvious for other engineers, so ideally, we would move OnboardingCheck into the root layout and trigger the onboarding modal based on the current path.
I don't think this is a blocker for merging though, so I'm approving. You can create a technical issue to do the refactor and assign it to me if you want. I'll handle it when there is time.
This reverts commit 12b8fd2.
- Onboarding "Remind Me Later" button suppresses modal for 1 day
…' into feat/1454-onboarding-persistence
# Conflicts: # front_end/src/app/(main)/components/mobile_menu.tsx
…' into feat/1454-onboarding-persistence
Introduced a UX improvement to prevent the onboarding screen from appearing if the user skips it on the previous page. - - This resolves cases where a user skips onboarding on the homepage, navigates to the questions feed, and sees the onboarding screen again.
?start_onboarding=truetriggercloses #1454