-
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
refactor: images-loading #80
Conversation
WalkthroughThis pull request updates the application’s hero image handling and configuration. The main layout now preloads hero images using a new GraphQL query and utility function. Several Hero components have been modified to use the Changes
Sequence Diagram(s)sequenceDiagram
participant RL as RootLayout
participant Q as GraphQL Server
participant U as getHeroImgsProps
participant HD as Head Element
RL->>Q: Request hero images (herosImagesQuery)
Q-->>RL: Return hero images data
RL->>U: Call getHeroImgsProps(data)
U-->>RL: Return image properties array
RL->>HD: Inject <link> elements for preloading
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for kleros-website-v2 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/src/queries/heros.ts (2)
3-16
: Consider using GraphQL fragments for repeated structures.The query structure for both hero sections is identical. Using a fragment would make the query more maintainable.
export const herosQuery = gql` + fragment HeroBackground on Hero { + background { + url + } + } + { pnkTokenPageHero { - background { - url - } + ...HeroBackground } earnPageHero { - background { - url - } + ...HeroBackground } } `;
18-29
: Consider using a shared interface for common types.The background type is repeated. Using a shared interface would make the type definitions more maintainable.
+interface Background { + background: { + url: string; + }; +} + export type HeroQueryType = { - pnkTokenPageHero: { - background: { - url: string; - }; - }; - earnPageHero: { - background: { - url: string; - }; - }; + pnkTokenPageHero: Background; + earnPageHero: Background; };frontend/src/app/layout.tsx (2)
28-41
: Optimize image preloading implementation.A few suggestions to improve the image preloading implementation:
- Consider conditional rendering based on successful data fetch
- The
imageSizes
attribute might need adjustment for different viewports- Consider adding
fetchpriority="high"
for critical hero images<head> + {herosImgs && ( <link rel="preload" as="image" href={herosImgs.earnPageHero.background.url} - imageSizes="100vw" + imageSizes="(max-width: 768px) 100vw, 80vw" + fetchpriority="high" /> <link rel="preload" as="image" href={herosImgs.pnkTokenPageHero.background.url} - imageSizes="100vw" + imageSizes="(max-width: 768px) 100vw, 80vw" + fetchpriority="high" /> + )} </head>
26-49
: Consider implementing error boundaries.The component could benefit from error boundaries to handle rendering failures gracefully.
// Create an ErrorBoundary component class ErrorBoundary extends React.Component { // Implementation details... } // Wrap the main content <ErrorBoundary fallback={<div>Something went wrong</div>}> {/* Existing JSX */} </ErrorBoundary>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/app/layout.tsx
(2 hunks)frontend/src/queries/heros.ts
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - kleros-website-v2
- GitHub Check: Header rules - kleros-website-v2
- GitHub Check: Pages changed - kleros-website-v2
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.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/src/components/BrandAssets/Hero.tsx (1)
30-37
: LGTM! Consider optimizing image loading further.The addition of
sizes="100vw"
is appropriate for a full-viewport-width background image. The combination withfill
,priority
, and proper CSS classes ensures optimal rendering.Consider adding a
quality
prop to optimize the image further:<Image src={heroData.background.url} alt="Hero Image Background" fill priority sizes="100vw" + quality={85} className="absolute left-0 top-0 z-[-1] h-full object-cover" />
frontend/src/components/Community/hero.tsx (1)
33-40
: LGTM! Consider the same image optimization.The addition of
sizes="100vw"
is appropriate and consistent with other hero components.For consistency with other hero components, consider adding a
quality
prop:<Image src={background.url} alt="Hero Image Background" fill priority sizes="100vw" + quality={85} className="absolute left-0 top-0 z-[-1] h-full object-cover" />
frontend/src/components/ForBuilders/Hero.tsx (1)
45-52
: LGTM! Consider the same image optimization.The addition of
sizes="100vw"
is appropriate and consistent with other hero components.For consistency with other hero components, consider adding a
quality
prop:<Image src={heroData.background.url} alt="Hero Image Background" fill priority sizes="100vw" + quality={85} className="absolute left-0 top-0 z-[-1] h-full object-cover" />
frontend/next.config.mjs (1)
7-12
: Consider using environment variables for the hostname.While the remote pattern is correctly configured for the Strapi media server, it would be more maintainable to use an environment variable for the hostname.
remotePatterns: [ { protocol: "https", - hostname: "delicate-eggs-595f0e58b8.media.strapiapp.com", + hostname: process.env.NEXT_PUBLIC_STRAPI_MEDIA_HOST, }, ],Add to your
.env
file:NEXT_PUBLIC_STRAPI_MEDIA_HOST=delicate-eggs-595f0e58b8.media.strapiapp.com
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/next.config.mjs
(1 hunks)frontend/src/components/BrandAssets/Hero.tsx
(1 hunks)frontend/src/components/Community/hero.tsx
(1 hunks)frontend/src/components/ForBuilders/Hero.tsx
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - kleros-website-v2
- GitHub Check: Header rules - kleros-website-v2
- GitHub Check: Pages changed - kleros-website-v2
🔇 Additional comments (1)
frontend/next.config.mjs (1)
6-6
: Well-chosen responsive breakpoints!The device sizes array provides a good range of breakpoints from mobile (640px) to large desktop (1920px) displays, which will help optimize image loading across different devices.
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.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
frontend/src/app/for-lawyers/components/Hero.tsx (1)
13-15
:⚠️ Potential issueAdd error handling for the GraphQL request.
The hero data fetch could fail. Consider adding error handling to gracefully handle failures.
- const { header, subtitle, buttons, arrowLink, background } = ( - await request<HeroQueryType>(heroQuery) - ).forLawyersPageHero; + let heroData; + try { + heroData = await request<HeroQueryType>(heroQuery); + } catch (error) { + console.error('Failed to fetch hero data:', error); + // Consider implementing a fallback UI + return null; + } + const { header, subtitle, buttons, arrowLink, background } = heroData.forLawyersPageHero;
♻️ Duplicate comments (1)
frontend/src/app/layout.tsx (1)
26-26
:⚠️ Potential issueAdd error handling for the GraphQL request.
The GraphQL request could fail. Consider adding error handling to gracefully handle failures.
🧹 Nitpick comments (1)
frontend/src/app/layout.tsx (1)
27-32
: Consider preloading all hero images.While preloading the earn page hero is good, consider preloading other hero images from
herosImgs
as well, since they're already fetched.const props = getImageProps({ src: herosImgs.earnPageHero.background.url, alt: "earn", fill: true, priority: true, }); +const pnkProps = getImageProps({ + src: herosImgs.pnkTokenPageHero.background.url, + alt: "pnk", + fill: true, + priority: true, +}); return ( <html lang="en"> <head> <link rel="preload" as="image" imageSrcSet={props.props.srcSet} imageSizes="100vw" /> + <link + rel="preload" + as="image" + imageSrcSet={pnkProps.props.srcSet} + imageSizes="100vw" + /> </head>Also applies to: 36-43
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/next.config.mjs
(1 hunks)frontend/src/app/for-lawyers/components/Hero.tsx
(1 hunks)frontend/src/app/layout.tsx
(2 hunks)frontend/src/components/Cooperative/hero.tsx
(1 hunks)frontend/src/components/Earn/Hero.tsx
(1 hunks)frontend/src/components/PNKToken/Hero.tsx
(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/src/components/PNKToken/Hero.tsx
- frontend/src/components/Earn/Hero.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - kleros-website-v2
- GitHub Check: Header rules - kleros-website-v2
- GitHub Check: Pages changed - kleros-website-v2
🔇 Additional comments (5)
frontend/next.config.mjs (2)
5-5
: Verify the cache duration setting.The
minimumCacheTTL
is set to 31536000 seconds (365 days). While this maximizes caching benefits, consider if such a long duration is appropriate for your use case, especially if images might need updates.
6-6
: LGTM! Device sizes are well-configured.The
deviceSizes
array includes common breakpoints (640px to 1920px) that align well with responsive design best practices.frontend/src/app/for-lawyers/components/Hero.tsx (1)
44-44
: LGTM! Responsive image sizing is well-configured.The
sizes="100vw"
attribute is correctly set, ensuring the image spans the full viewport width and loads the appropriate size based on the device.frontend/src/components/Cooperative/hero.tsx (2)
46-46
: LGTM! Responsive image sizing is well-configured.The
sizes="100vw"
attribute is correctly set, ensuring optimal image loading based on viewport width.
11-16
: Great use of props pattern!The component receives hero data as props instead of fetching it directly, making it more reusable and easier to test.
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.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/app/layout.tsx (1)
25-26
:⚠️ Potential issueAdd error handling for the GraphQL request.
The GraphQL request could fail. Consider adding error handling to gracefully handle failures.
🧹 Nitpick comments (6)
frontend/src/utils/getHeroImgsProps.ts (1)
5-17
: Add JSDoc documentation for better code maintainability.Consider adding JSDoc documentation to describe the function's purpose, parameters, and return type.
+/** + * Generates image properties for hero components using Next.js's getImageProps. + * @param data - Hero images data from GraphQL query + * @returns Array of image properties for preloading + */ export const getHeroImgsProps = (data: HeroImagesQueryType) => {frontend/src/queries/heroImages.ts (2)
3-54
: Use GraphQL fragments to reduce code duplication.The query repeats the same fields for each hero. Consider using a fragment to make the code more maintainable.
export const herosImagesQuery = gql` + fragment HeroBackground on Hero { + background { + name + url + } + } + { earnPageHero { - background { - name - url - } + ...HeroBackground } forBuildersPageHero { - background { - name - url - } + ...HeroBackground } # ... apply the same pattern to other heroes } `;
55-57
: Add JSDoc documentation for types.Consider adding JSDoc documentation to describe the types and their purpose.
+/** + * Represents a hero component with background image. + */ type Hero = { background: { name: string; url: string }; }; +/** + * Type representing the response from the hero images GraphQL query. + */ export type HeroImagesQueryType = {Also applies to: 59-68
frontend/src/app/layout.tsx (1)
30-41
: Optimize image sizes for different viewports.The
imageSizes
attribute is hardcoded to "100vw". Consider using responsive sizes to optimize image loading for different viewports.- imageSizes="100vw" + imageSizes="(max-width: 768px) 100vw, (max-width: 1200px) 80vw, 1400px"frontend/src/components/ResearchDevelopment/Hero.tsx (2)
47-53
: Improve image accessibility with descriptive alt text.The current alt text "Hero Image Background" is generic. Consider using a more descriptive alt text that reflects the content or purpose of the image.
- alt="Hero Image Background" + alt={`Research & Development Hero - ${background.name}`}
47-53
: Use CSS isolation for z-index stacking context.Using negative z-index values can lead to stacking context issues. Consider creating a new stacking context using CSS isolation.
- className="absolute left-0 top-0 z-[-1] h-full object-cover" + className="absolute left-0 top-0 isolate -z-10 h-full object-cover"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
frontend/src/app/for-lawyers/components/Hero.tsx
(1 hunks)frontend/src/app/layout.tsx
(2 hunks)frontend/src/components/BrandAssets/Hero.tsx
(1 hunks)frontend/src/components/Community/hero.tsx
(1 hunks)frontend/src/components/Cooperative/hero.tsx
(1 hunks)frontend/src/components/Earn/Hero.tsx
(1 hunks)frontend/src/components/ForBuilders/Hero.tsx
(1 hunks)frontend/src/components/PNKToken/Hero.tsx
(1 hunks)frontend/src/components/ResearchDevelopment/Hero.tsx
(1 hunks)frontend/src/queries/heroImages.ts
(1 hunks)frontend/src/utils/getHeroImgsProps.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- frontend/src/components/BrandAssets/Hero.tsx
- frontend/src/components/PNKToken/Hero.tsx
- frontend/src/components/Cooperative/hero.tsx
- frontend/src/components/Community/hero.tsx
- frontend/src/components/Earn/Hero.tsx
- frontend/src/app/for-lawyers/components/Hero.tsx
- frontend/src/components/ForBuilders/Hero.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - kleros-website-v2
- GitHub Check: Header rules - kleros-website-v2
- GitHub Check: Pages changed - kleros-website-v2
🔇 Additional comments (1)
frontend/src/utils/getHeroImgsProps.ts (1)
5-17
: Verify the impact of disabling image optimization.Setting
unoptimized: true
disables Next.js's built-in Image Optimization API. This could affect image loading performance and optimization. Please ensure this aligns with your performance requirements.✅ Verification successful
🌐 Web query:
What are the implications of using unoptimized images in Next.js?
💡 Result:
Using unoptimized images in Next.js introduces several performance and user experience drawbacks that impact both developers and end-users:
Core Implications
1. Increased Page Load Times
Unoptimized images retain their original file sizes, leading to:
- 4-10x larger payloads compared to WebP/AVIF formats [7]
- Slower Largest Contentful Paint (LCP) metrics [1][5]
- No automatic viewport-based lazy loading [5][6]
2. Layout Instability
Without explicit dimensions:
- Cumulative Layout Shift (CLS) increases as images load [1][5]
- Browser struggles to reserve space before image rendering [2][9]
3. Bandwidth Waste
Serving full-resolution assets to all devices:
- Mobile users download desktop-sized images [7][9]
- No responsive
srcset
generation ([6] shows default srcset behavior vs unoptimized)4. SEO Penalties
Search engines penalize for:
- Poor Core Web Vitals scores (LCP/CLS) [1][7]
- Missing modern format optimizations [5][10]
- Excessive page weight [9]
Technical Comparison
Optimized (Next.js Image) Unoptimized ( <img>
)Automatic WebP/AVIF conversion Original formats only Device-specific resizing Single size for all devices Lazy loading by default Eager loading unless manual CLS prevention via aspect ratio Layout shifts common ~30KB avg size (4K image) ~4MB avg size (4K image) [7] Implementation Consequences
When using
unoptimized={true}
([4][6]):// Disables all Next.js optimizations <Image src="/large.jpg" width={1200} height={800} unoptimized />This configuration:
- Bypasses Sharp optimization pipeline [3]
- Disables automatic quality reduction [6]
- Removes format negotiation logic [6]
- Increases server load from raw asset delivery [9]
Developers must manually implement equivalent optimizations through third-party tools or CDNs when opting out of Next.js' built-in image handling[8][10]. The framework's automatic on-demand optimization ([5] shows 75% size reduction typically) becomes unavailable, requiring significant additional work to match native performance characteristics.
Citations:
- 1: https://nextjs.org/docs/pages/building-your-application/optimizing/images
- 2: https://uploadcare.com/blog/image-optimization-in-nextjs/
- 3: https://dev.to/vsnikhilvs/nextjs-image-optimization-c3p
- 4: https://uploadcare.com/blog/next-js-image-optimization/
- 5: https://nextjs.org/learn/seo/images
- 6: https://nextjs.org/docs/pages/api-reference/components/image
- 7: https://imagekit.io/blog/nextjs-image-optimization/
- 8: https://www.upheave.tech/blog/optimizing-images-in-nextjs/
- 9: https://currentior.com/optimizing-images-in-next-js-a-comprehensive-guide/
- 10: https://refine.dev/blog/using-next-image/
Assessment of Disabling Next.js Image Optimization
The use of
unoptimized: true
bypasses Next.js’s built-in image handling. As confirmed by recent findings:
- Increased File Sizes & Load Times: Images retain their original sizes, leading to slower Largest Contentful Paint (LCP) and higher bandwidth usage.
- Layout Instability & CLS: Without automated resizing and lazy loading, layout shifts can increase, adversely impacting Core Web Vitals.
- SEO Implications: Poor performance and larger page weights may negatively affect SEO.
- Manual Optimization Requirement: You’ll need to implement alternative optimizations (e.g., external CDNs or manual processing) to mitigate these issues if performance is a priority.
Please verify that these trade-offs align with your performance requirements before proceeding.
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.
lgtm
WIP
Summary by CodeRabbit
New Features
Refactor
Chores