[Onboarding] - Refactor tabs and routing #5184
Conversation
✅ Deploy Preview for dev-web-novu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| <Route path={ROUTES.GET_STARTED} element={<GetStartedPage />} /> | ||
| ) : ( | ||
| <Route path={ROUTES.GET_STARTED} element={<GetStarted />} /> | ||
| )} |
There was a problem hiding this comment.
This conditional (that requires a re-render) is why search params don't persist across refresh. Once we remove this, they will work as expected
There was a problem hiding this comment.
Not related to this scope
I wonder if we could/want to create a "loading" indication on our FF's so in such cases we would create a loading state, so we would avoid unnecessary rerenders and screen changes.
@antonjoel82 would love to hear your thoughts on it :)
There was a problem hiding this comment.
@djabarovgeorge What is FF in this context?
Sounds like a good UX improvement! We could do a temporary one, but overall my preference would be to upgrade our React version to use Suspense which will be a huge improvement overall.
Alternatively, one way I've done this in the past to avoid the re-routing issue is to route to a container page (just a page component that uses the hook to determine which content to show). Then, we'd delete that page later and point directly to the new functionality
There was a problem hiding this comment.
Sorry by i meant feature flag by FF.
Still had not the chance to use Suspense wonder how good is it.
Agree that a middleware component could help.
thanks for the feedback 🙃
antonjoel82
force-pushed
the
NV-3243_onboarding-in-app
branch
from
336aaa5
to
b7b0728
Compare
|
@djabarovgeorge This is the branch / PR I would branch off of! |
|
|
||
| const PAGE_TITLE = 'Get started'; | ||
|
|
||
| export function GetStartedPage() { | ||
| const { currentOrganization } = useAuthContext(); | ||
| const { currentTab, setTab } = useTabSearchParams(); |
There was a problem hiding this comment.
this seems like should be used inside the GetStartedTabs:
- otherwise, it will rerender this whole component (but it's fine in this case)
- child controls the parent state (nah)
There was a problem hiding this comment.
Thanks for the feedback! I will merge it as is for now, but we can memoize the tab behaviors so that they don't re-render the whole component tree if it becomes necessary.
antonjoel82
force-pushed
the
usecase-onboarding-project
branch
from
0ed134c
to
d9c3bf0
Compare
antonjoel82
force-pushed
the
NV-3243_onboarding-in-app
branch
from
b7b0728
to
b4d29fd
Compare
What change does this PR introduce?
GetStartedTaba dumb component that just accepts a rendering logic from propsWhy was this change needed?
Other information (Screenshots)
https://www.loom.com/share/67bc4170b0fa439087521d1730a95222