Enhance load on home screen #94
Conversation
| @@ -36,18 +36,19 @@ export default function RootLayout({ children }: { children: React.ReactNode }) | |||
| return ( | |||
| <html lang="en"> | |||
| <head> | |||
| <Script async strategy="afterInteractive" src="https://www.googletagmanager.com/gtag/js?id=G-ZSC2EVZSVD" /> | |||
| <Script | |||
There was a problem hiding this comment.
@mattystank Since this is not a dynamic script, and we trust the source (Google), we can 'safely' use dangerouslySetInnerHTML, but, sanitization would be best practice. Can you add a ticket to the backlog to sanitize the response?
src/components/home/SiteHomeRows.tsx
Outdated
| const maxWidth: number = 350 | ||
| const rowPaddingBottom: number = 64 | ||
| const industryTestingResourceRow: number = 350 | ||
| const maxWidth = useMemo(() => 350, []) |
There was a problem hiding this comment.
IMO, maxWidth, rowPaddingBottom, and industryTestingResourceRow should all be changed back to:
const maxWidth: number = 350
const rowPaddingBottom: number = 64
const industryTestingResourceRow: number = 350
This is because useMemo() would actually slow down the load in these cases. You see, useMemo is used to store the result of complex calculations so that they don't have to be run again. But, in this case, there is no calculation, it's just an integer being assigned, and, as a constant, which will never change anyway. So, I fear that useMemo is both unclear and reduce performance by introducing extra overhead calling the function. In addition, useMemo wouldn't help with load because it can only save the result after it's been run at least once.
| }} | ||
| src={ONCLogo} | ||
| alt="ONC Logo" | ||
| loading="lazy" |
There was a problem hiding this comment.
I'm assuming you made this lazy in order to help ensure the cards load faster/first and thus show up before the logo, correct? Since they are part of the user experienc/navigation.
|
|
||
| useEffect(() => { | ||
| // Simulate a loading state | ||
| const timer = setTimeout(() => setLoading(false), 300) // Example loading time |
There was a problem hiding this comment.
Althoug this will help the percieved loading time, it is based on a semi-arbitrary/unknown (changing in the future) value of 300ms. During that time, we are essentially blocking the UI. Sure, it's async, but, by having a specific value, we are not displaying the content as soon as it has been fetched. Instead, we wait for the timer which guesses it. So, we are potentially introducing a longer wait overall. How it should work, IMO, is that it the useEffect should trigger based on not a pre-determined time, but on the state of the data being fetched in the carousel. Either way, it is a good start, so, thanks for that!
We could also consider in the future:
-SSR if possible
-Using the skeletons at a more specifc level within the cards
-react-multi-carousel library optimization
-And as discussed prior, lazy loading the cards we can't see in the carousel (and loading from top to bottom in general, for the 3 rows of cards)
There was a problem hiding this comment.
Note to self: We can also try code splitting (React.lazy() for a specific card along with Suspense to target it) and apply that to the cards which aren't seen on startup
There was a problem hiding this comment.
Overall, we can merge this.
We just have to be aware that it is more a work in progress than a final result. We need 2 new tickets. One to address the sanitization, and one to update the loading state to trigger based off the actual application state vs a timer. The latter ticket can also be to expand on all of the possible options I mentioned to further improve the load. Thanks, man!
That being said, let's remove useMemo from the 3 consts I mentioned before the merge if you don't mind. Thanks again!
useMemo is removed, let me know if this okay to merge after my last commit |
There was a problem hiding this comment.
Merging. Just a note, I only mentioned the 3 consts to remove useMemo from because it could have helped performance potentially on the other items, especially the one with theme. Not on first load, sure, but subsequent loads. Anyway, we can always copy it back in if needed.
No description provided.