-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: do not show sidebar while loading #387
Conversation
works. but still it shows blank white screen and then goes to login screen. but maybe it's too complex to handle state or show something consistent there while things are loading. |
@@ -91,6 +91,10 @@ export default function AppLayout() { | |||
return null; | |||
} | |||
|
|||
if (!info.setupCompleted || !info.running || !info.unlocked) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks unsafe to me. What about we move this info check
if (!info) {
return null;
}
Into App.tsx e.g.
{info && <RouterProvider router={router} />}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 yup that would be good, but line 94 does the fix here, should we also move it to App.tsx is what you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 94 with any of these conditions not being true feels worrying to me - it could break one of our pages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if any of it is false, we do the redirect in HomeRedirect
. Outlet
is anyways rendered even now. But it is just rendered with the Sidebar. (And it consists of the HomeRedirect
component which then transfers to the intro/start/unlock pages) This change just removes the Sidebar in those cases by rendering just the Outlet
(HomeRedirect
component)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@im-adithya this makes it more complicated and does not cleanly solve the problem. We don't even know what page to render if the info is not loaded. Rendering the outlet makes no sense
This change won't fix the issue, we somehow have to change the HomeRedirect or render only the Outlet but not the Sidebar when HomeRedirect is deciding which page to render This is on current master 7d4e7b6 Screen.Recording.2024-08-09.at.8.18.19.PM.mov |
This should fix the loading issue when node is unlocked/not running