-
Notifications
You must be signed in to change notification settings - Fork 40
Performance Improvement: LCP Optimization via Critical HTML Rendering #123
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: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,17 @@ | ||
| import { StrictMode } from 'react' | ||
| import { createRoot } from 'react-dom/client' | ||
| import './index.css' | ||
| import App from './App.tsx' | ||
| import { StrictMode } from "react"; | ||
| import { createRoot } from "react-dom/client"; | ||
| import "./index.css"; | ||
| import App from "./App"; | ||
|
|
||
| createRoot(document.getElementById('root')!).render( | ||
| const root = createRoot(document.getElementById("root")!); | ||
|
|
||
| root.render( | ||
| <StrictMode> | ||
| <App /> | ||
| </StrictMode>, | ||
| ) | ||
| </StrictMode> | ||
| ); | ||
|
|
||
| const staticLCP = document.getElementById("lcp-static"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Suggestion — The logic to remove the 'lcp-static' element after rendering is not commented, which may confuse future maintainers about the purpose of this removal. Add a comment explaining why the 'lcp-static' element is removed after rendering, e.g., 'Remove static LCP placeholder after React app mounts to improve performance'. readability There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Suggestion — Use of non-null assertion operator (!) on document.getElementById('root') without null check. Add a runtime check to ensure the element exists before using it to avoid potential runtime errors in environments where 'root' might be missing. best-practices There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Suggestion — The code removes the element with id 'lcp-static' immediately after rendering the React app. If the React app rendering fails or is delayed, the static LCP content might be removed prematurely, potentially causing a flash of empty content or layout shift. Consider removing the static LCP element only after the React app has fully hydrated or rendered successfully, possibly by placing the removal inside a React effect or after confirming the app is ready. bugs |
||
| if (staticLCP) { | ||
| staticLCP.remove(); | ||
| } | ||
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.
💡 Suggestion — The variable name 'staticLCP' is somewhat unclear and inconsistent with the rest of the codebase's camelCase convention for variables. It is not immediately obvious what 'LCP' stands for without context.
Rename 'staticLCP' to a more descriptive name like 'lcpStaticElement' or 'lcpStaticSection' to improve clarity and maintain consistency with camelCase naming.
readability