-
Notifications
You must be signed in to change notification settings - Fork 0
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
Base page refactor #99
Conversation
Preview is no longer available for this pull request. |
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.
I updated the lazy-loading strategy to also consider Base page. Other than that, I think it is good to be merged in
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.
Good refactoring to take out a base page, I think it will serve us well in the long run.
Not 100% sure about lazy loading at the web app level. I though the purpose of lazy loading was that it would allow us to show something to the user right away as an encouragement (like the navbar) and then show the content of the page. What is this desired behavior? Anyways things look so fast it's hard to tell what the effects are. I trust @alexnguyenn's and @Keelando 's judgement on it.
@@ -6,7 +6,7 @@ const Base = (props: React.PropsWithChildren<Record<never, never>>) => { | |||
return ( | |||
<Container maxW="container.xl"> | |||
<Navbar /> | |||
<React.Suspense fallback={<></>}>{props.children}</React.Suspense> |
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.
@alexnguyenn I thought it would have made sense to have the lazy loading at the page level because we don't need a page to load until we actually navigate to it. Are al routes loaded on page load? or there is a second layer of lazy loading in the routes?
Also not sure how all of this plays differently between the React DOM and the actual DOM
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.
And I also thought loading the navbar first and then lazily loading the rest would have given the user confidence, by first showing the navbar
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.
I updated it a little bit in e711d6e - you might have missed that:
Lines 21 to 31 in e711d6e
<Base> | |
<React.Suspense fallback={<></>}> | |
<Routes> | |
<Route path="/" element={<Home />} /> | |
<Route path="/online-compiler" element={<CompilerPage />} /> | |
<Route path="/overview" element={<Overview />} /> | |
<Route path="/about-us" element={<AboutUs />} /> | |
<Route path="*" element={<NotFound />} /> | |
</Routes> | |
</React.Suspense> | |
</Base> |
So we are lazy-loading the content of each page (the props.children
passing into Base
), but not the Base
component. This means that components in Base
like Navbar
(or maybe later Footer
) will not be lazy-loaded. So what you mentioned above is already implemented - tho I might have missed something.
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.
You are right Base is outside of the routes. That is desired behavior
Refactor Base component from each page
Scrap creation of Compiler component.
Avoid lint errors accepting components as props with React.PropsWithChildren<Record<never, any>>