-
Notifications
You must be signed in to change notification settings - Fork 129
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 Home page and replace current index /quests #962
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@baitcode is attempting to deploy a commit to the LFG Labs Team on Vercel. A member of the Team first needs to authorize it. |
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe pull request introduces a new Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
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: 3
🧹 Outside diff range and nitpick comments (1)
app/quests/page.tsx (1)
52-58
: Consider making statistics dynamic.The statistics are currently hardcoded. These values should ideally be fetched from an API or provided through context to ensure they stay up-to-date.
-<Stats - stats={[ - { name: "Quests NFT minted", value: "+1M" }, - { name: "Unique addresses", value: "398K" }, - { name: "Unique visitors", value: "+200K" }, - ]} -/> +<Stats stats={statsFromContext} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/quests/page.tsx
(1 hunks)
🔇 Additional comments (2)
app/quests/page.tsx (2)
1-12
: LGTM! Clean imports with proper client-side directive.
The imports are well-organized and the "use client" directive is correctly placed at the top of the file.
19-23
: LGTM! Clean layout structure with proper styling.
The layout is well-organized with appropriate use of CSS modules and decorative elements (Blur components).
Also applies to: 59-61
- added new card component, header and split header - implemented style - fixed css for navbar - added card images - added cut word for split effect - extended tailwinf with padding needed (should not be doing this, but realized too late that tailwind is conflicting)
Implemented EARN animation
* removed TODO * implemented animations
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: 3
🧹 Outside diff range and nitpick comments (2)
components/pages/home/splitTitle.tsx (2)
351-357
: Enhance type documentation with JSDoc commentsWhile the type properties have inline comments, using JSDoc would provide better IDE support and documentation generation.
type SplitTitleProps = { + /** Additional class for component container */ className?: string; + /** Value in pixels identifying the radius of splits to fly around */ spread?: number; + /** Amount of seconds fade duration will take */ fadeDuration?: number; + /** Child staggering amount (small timeout before every children that offsets animation start) */ stagger?: number; + /** Amount of seconds between fadeIn animation finish and fadeOut animation start */ interval?: number; };
21-349
: Consider code-splitting SVG constantsThe SVG constants contain large amounts of path data which could impact initial bundle size. Consider moving these to a separate file and potentially lazy-loading them.
You could:
- Move the SVG data to a separate file (e.g.,
splitTitlePaths.ts
)- Use dynamic imports to load the paths when needed
- Show a simpler placeholder during the loading state
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/pages/home/splitTitle.tsx
(1 hunks)
🔇 Additional comments (2)
components/pages/home/splitTitle.tsx (2)
394-403
: Add missing dependency array to useEffect
This issue was already identified in a past review comment. The useEffect hook is missing a dependency array, which can cause unnecessary re-renders and multiple timeouts.
373-392
: Use consistent random values for animations
This issue was already identified in a past review comment. Using Math.random() directly in the variants can cause unpredictable animations and make testing difficult.
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.
Have a look at AI suggestion, Code doesn't compile, please make sure it compiles before sending the PR
app/page.tsx
Outdated
|
||
return ( | ||
<div className={styles.screen}> | ||
<div className={styles.container}> | ||
<div className={`flex flex-col items-center justify-center`}> |
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.
delete {``} to use ""
splits: Array<ClipPath> // Polygon ClipPaths describing shattered shapes | ||
} | ||
|
||
const svgAttributes: Record<string, SvgSettings> = { |
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.
To add in another file as an SVG to /icons
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.
Pardon. I do not understand what are you trying to say.
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: 5
🧹 Outside diff range and nitpick comments (9)
components/pages/home/card.tsx (1)
7-13
: Add JSDoc documentation for the CardProps type.Consider adding JSDoc documentation to improve code maintainability and IDE support.
+/** + * Props for the Card component + * @property {React.ReactNode} children - Child elements to render inside the card + * @property {string} imgSrc - URL of the card's background image + * @property {string} title - Title text for the card + * @property {() => void} onClick - Click handler function + * @property {boolean} [disabled] - Whether the card is disabled + */ type CardProps = { children: React.ReactNode; imgSrc: string; title: string; onClick: () => void; disabled?: boolean; };eslint.config.mjs (2)
24-27
: Reconsider globally disabled Next.js rulesDisabling
@next/next/no-img-element
globally might lead to accessibility and performance issues. Consider:
- Using
next/image
for optimized images- Only disabling these rules in specific files where necessary
rules: { - "@next/next/no-img-element": "off", + "@next/next/no-img-element": "warn", // Consider enabling and fixing issues "@next/next/no-document-import-in-page": "off", },
1-41
: Consider adding modern ESLint featuresThe configuration could benefit from these modern ESLint features:
- Explicit file ignores
- Linting performance optimization
+// Add at the beginning of the configuration array +{ + ignores: [ + '**/node_modules/**', + '**/dist/**', + '**/build/**', + '**/.next/**' + ] +}, +{ + linterOptions: { + reportUnusedDisableDirectives: true, + noInlineConfig: true + } +},components/pages/home/hero.tsx (2)
1-1
: Remove unused import.The
FunctionComponent
type is imported but never used since the component usesReact.FC
.-import React, { FunctionComponent } from "react"; +import React from "react";🧰 Tools
🪛 eslint
[error] 1-1: 'FunctionComponent' is defined but never used.
(@typescript-eslint/no-unused-vars)
23-23
: Escape the apostrophe in the text content.The apostrophe in "Starknet's" should be escaped to ensure consistent rendering across different environments.
- for individuals, DAOs, and protocols to grow their assets and explore Starknet's + for individuals, DAOs, and protocols to grow their assets and explore Starknet's🧰 Tools
🪛 eslint
[error] 23-23:
'
can be escaped with'
,‘
,'
,’
.(react/no-unescaped-entities)
hooks/useGetDiscoveryWallets.ts (2)
Line range hint
4-42
: Add error handling and improve type safetyThe current implementation has several areas that could be improved:
- Error handling for the Promise
- Type safety for browser-specific downloads
- Loading state management
Consider applying these improvements:
export default function useGetDiscoveryWallets( getDiscoveryWallets: Promise<WalletProvider[]> ) { const [argentX, setArgentX] = useState<string>(""); const [braavos, setBraavos] = useState<string>(""); const [bitkeep, setBitkeep] = useState<string>(""); + const [isLoading, setIsLoading] = useState(true); + const [error, setError] = useState<Error | null>(null); useEffect(() => { if (typeof navigator !== "undefined") { - getDiscoveryWallets.then((wallets) => { - const browser = getBrowser(navigator.userAgent); + setIsLoading(true); + getDiscoveryWallets + .then((wallets) => { + const browser = getBrowser(navigator.userAgent); + if (!browser) { + throw new Error("Unsupported browser detected"); + } - wallets.map((wallet) => { - if (wallet.id === "argentX") { - setArgentX( - browser - ? wallet.downloads[browser as keyof typeof wallet.downloads] - : "https://www.argent.xyz/argent-x/" - ); - } else if (wallet.id === "braavos") { - setBraavos( - browser - ? wallet.downloads[browser as keyof typeof wallet.downloads] - : "https://braavos.app/download-braavos-wallet/" - ); - } else if (wallet.id === "bitkeep") { - setBitkeep( - browser - ? wallet.downloads[browser as keyof typeof wallet.downloads] - : "https://web3.bitget.com/en/wallet-download" - ); - } - }); - }); + wallets.forEach((wallet) => { + const downloads = wallet.downloads; + if (!downloads || !(browser in downloads)) { + throw new Error(`No download URL found for ${wallet.id} on ${browser}`); + } + + switch (wallet.id) { + case "argentX": + setArgentX(downloads[browser] ?? "https://www.argent.xyz/argent-x/"); + break; + case "braavos": + setBraavos(downloads[browser] ?? "https://braavos.app/download-braavos-wallet/"); + break; + case "bitkeep": + setBitkeep(downloads[browser] ?? "https://web3.bitget.com/en/wallet-download"); + break; + } + }); + }) + .catch((err) => { + setError(err); + console.error("Failed to fetch wallet information:", err); + }) + .finally(() => { + setIsLoading(false); + }); } }, [getDiscoveryWallets]); - return { argentX, braavos, bitkeep }; + return { argentX, braavos, bitkeep, isLoading, error };
Line range hint
4-42
: Consider extracting configuration to a separate fileThe hardcoded fallback URLs should be moved to a configuration file to improve maintainability.
Create a new configuration file
config/wallets.ts
:export const WALLET_CONFIGS = { argentX: { defaultUrl: "https://www.argent.xyz/argent-x/", id: "argentX" }, braavos: { defaultUrl: "https://braavos.app/download-braavos-wallet/", id: "braavos" }, bitkeep: { defaultUrl: "https://web3.bitget.com/en/wallet-download", id: "bitkeep" } } as const;Then update the hook to use these configurations.
app/page.tsx (1)
15-22
: Consider consolidating navigation handlers.The navigation handlers could be consolidated into a single reusable function to reduce code duplication.
Here's a suggested implementation:
- const handleNavigateToDefi = useCallback(() => { - router.push("/discover/defi"); - }, [router]); - - const handleNavigateToQuests = useCallback(() => { - router.push("/quests"); - }, [router]); + const handleNavigate = useCallback((path: string) => { + router.push(path); + }, [router]);Then update the onClick handlers:
onClick={() => handleNavigate("/discover/defi")} onClick={() => handleNavigate("/quests")}components/pages/home/splitTitle.tsx (1)
396-396
: Use 'const' instead of 'let' for 'timeout' variableThe variable
timeout
is never reassigned after its initial declaration. Usingconst
instead oflet
signals that this variable should not change, improving code readability and maintainability.Apply this diff to fix the issue:
@@ -396,7 +396,7 @@ // This effects allow for initial interval timeout - let timeout = setTimeout(() => { + const timeout = setTimeout(() => { setFirstLaunch(false); }, interval * 1000); @@ -422,7 +422,7 @@ } else { - let timeout = setTimeout(() => setVisible(true), interval * 1000) + const timeout = setTimeout(() => setVisible(true), interval * 1000) return () => { window.clearTimeout(timeout) }; }Also applies to: 422-422
🧰 Tools
🪛 eslint
[error] 396-396: 'timeout' is never reassigned. Use 'const' instead.
(prefer-const)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
app/page.tsx
(1 hunks)components/UI/changeWallet.tsx
(1 hunks)components/UI/navbar.tsx
(2 hunks)components/pages/home/card.tsx
(1 hunks)components/pages/home/hero.tsx
(1 hunks)components/pages/home/splitTitle.tsx
(1 hunks)eslint.config.mjs
(1 hunks)hooks/useGetDiscoveryWallets.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/UI/changeWallet.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- components/UI/navbar.tsx
🧰 Additional context used
📓 Learnings (1)
components/pages/home/splitTitle.tsx (2)
Learnt from: baitcode
PR: lfglabs-dev/starknet.quest#962
File: components/pages/home/splitTitle.tsx:3-3
Timestamp: 2024-11-27T07:26:15.265Z
Learning: In the file `components/pages/home/splitTitle.tsx`, the `motion` library is correctly imported from `motion/react` rather than `framer-motion`.
Learnt from: baitcode
PR: lfglabs-dev/starknet.quest#962
File: components/pages/home/splitTitle.tsx:373-392
Timestamp: 2024-11-27T07:25:07.612Z
Learning: In the `SplitTitle` component (`components/pages/home/splitTitle.tsx`), the use of `Math.random()` in the `getVariants` function is intentional to create unpredictable animations.
🪛 eslint
components/pages/home/card.tsx
[error] 1-1: 'FunctionComponent' is defined but never used.
(@typescript-eslint/no-unused-vars)
components/pages/home/hero.tsx
[error] 1-1: 'FunctionComponent' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 23-23: '
can be escaped with '
, ‘
, '
, ’
.
(react/no-unescaped-entities)
components/pages/home/splitTitle.tsx
[error] 396-396: 'timeout' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 422-422: 'timeout' is never reassigned. Use 'const' instead.
(prefer-const)
🔇 Additional comments (10)
components/pages/home/card.tsx (1)
15-28
:
Enhance accessibility and user interaction.
The component needs improvements for better accessibility and user experience.
-const Card: React.FC<CardProps> = ({ children, title, imgSrc, onClick, disabled }) => {
+const Card: React.FC<CardProps> = ({ children, title, imgSrc, onClick, disabled }) => {
+ const handleKeyDown = (event: React.KeyboardEvent<HTMLDivElement>) => {
+ if (disabled) return;
+ if (event.key === 'Enter' || event.key === ' ') {
+ event.preventDefault();
+ onClick();
+ }
+ };
+
+ const [imageError, setImageError] = React.useState(false);
+
return (
<div
className={styles.card}
- onClick={onClick}
+ onClick={disabled ? undefined : onClick}
+ onKeyDown={handleKeyDown}
aria-disabled={disabled}
+ role="button"
+ tabIndex={disabled ? -1 : 0}
+ aria-label={`${title} card`}
>
<div
- style={{ backgroundImage: `url('${cdnize(imgSrc)}')` }}
+ style={{
+ backgroundImage: !imageError ? `url('${cdnize(imgSrc)}')` : 'none',
+ backgroundColor: imageError ? '#f0f0f0' : undefined
+ }}
className={styles.cardImage}
+ role="img"
+ aria-label={title}
+ onError={() => setImageError(true)}
/>
<div className={`${styles.cardInfos}`}>
<Typography
type={TEXT_TYPE.H3}
color="secondary"
className={styles.cardTitle}
+ id={`card-title-${title.toLowerCase().replace(/\s+/g, '-')}`}
>
{title}
</Typography>
{children}
</div>
</div>
);
};
Also ensure that the CSS includes proper focus styles:
.card:focus-visible {
outline: 2px solid #0066cc;
outline-offset: 2px;
}
eslint.config.mjs (2)
1-13
: LGTM: Imports and FlatCompat setup are well-structured
The imports are properly organized and the FlatCompat setup correctly uses the module's directory as the base.
35-40
: Verify the need to disable react/no-unknown-property
While disabling react/prop-types
is reasonable when using TypeScript, disabling react/no-unknown-property
might hide real issues. Let's verify the actual usage:
✅ Verification successful
After analyzing the codebase, I can now provide a final response regarding the need to disable the react/no-unknown-property
rule.
Disabling react/no-unknown-property is necessary for Three.js/React-Three-Fiber props
The rule disable is justified as the codebase uses React-Three-Fiber (R3F) and Three.js, which require several custom properties that ESLint's React plugin doesn't recognize by default:
- Custom WebGL properties in the
Canvas
component likegl={{ antialias, toneMapping, alpha }}
- Three.js-specific mesh properties like
attach="material"
,map
,transparent
,depthWrite
, etc. - R3F-specific props like
manual
in PerspectiveCamera
These properties are valid for Three.js/R3F components but would trigger false positives with the ESLint rule.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for custom attributes in land components that might trigger the rule
ast-grep --pattern 'JSXAttribute' components/lands/*.tsx
Length of output: 394
Script:
#!/bin/bash
# Search for JSX attributes in land components using ripgrep for better context
rg -U --type tsx "<[^>]+\s+[a-zA-Z]+=.*?>" components/lands/
Length of output: 90
Script:
#!/bin/bash
# Search for content of land components to analyze JSX usage
rg -l "\.tsx$" components/lands/ | xargs cat
Length of output: 46
Script:
#!/bin/bash
# First find the files
fd ".tsx$" components/lands/
# Then try to read their content
echo "=== Content of land components ==="
fd ".tsx$" components/lands/ -x cat {}
Length of output: 30654
components/pages/home/hero.tsx (2)
9-28
: Implementation follows best practices!
The Hero component is well-structured with:
- Semantic HTML and proper accessibility attributes
- Consistent use of the design system components
- Responsive design through utility classes
🧰 Tools
🪛 eslint
[error] 23-23: '
can be escaped with '
, ‘
, '
, ’
.
(react/no-unescaped-entities)
14-17
: Verify Typography component usage and text content.
The component uses Typography with different types (H1 and BODY_MICRO). Let's verify that these types exist and the text content matches the design specifications.
Also applies to: 20-25
✅ Verification successful
Typography component usage is correctly implemented
The Typography component in hero.tsx uses valid TEXT_TYPE values:
TEXT_TYPE.H1
for the main headingTEXT_TYPE.BODY_MICRO
for the descriptive text
Both types are properly defined in constants/typography.ts and the component implementation correctly handles these types with appropriate HTML tags (h1 and p respectively) and styling classes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Typography component types and usage
# Check Typography component for supported types
ast-grep --pattern 'export const TEXT_TYPE = {
$$$
}'
# Check for consistent usage of these Typography types across the app
ast-grep --pattern '<Typography type={TEXT_TYPE.$_}'
Length of output: 112
Script:
#!/bin/bash
# Let's try a different approach to find Typography related constants and components
# Search for Typography component definition and type declarations
rg -l "TEXT_TYPE" --type ts --type tsx
# Search for Typography component usage
rg "<Typography" --type tsx -A 2
# Search for any enum or constant declarations related to Typography
rg "export (type|const|enum).*(Typography|TEXT)" --type ts --type tsx
Length of output: 229
Script:
#!/bin/bash
# Let's try without file type specifications since tsx isn't recognized
# Search for Typography component definition and type declarations
rg -l "TEXT_TYPE"
# Search for Typography component usage
rg "<Typography" -A 2
# Search for any enum or constant declarations related to Typography
rg "export (type|const|enum).*(Typography|TEXT)"
# Also search for the specific file we're reviewing
fd "hero.tsx"
Length of output: 54138
Script:
#!/bin/bash
# Let's check the TEXT_TYPE definition to verify the types used in hero.tsx
# Check the content of typography constants
cat constants/typography.ts
# Also check the Typography component implementation
cat components/UI/typography/typography.tsx
Length of output: 2488
hooks/useGetDiscoveryWallets.ts (1)
2-2
: Verify package migration across the codebase
The import statement has been updated from get-starknet-core
to @starknet-io/get-starknet-core
.
Let's verify this change is consistent across the codebase:
✅ Verification successful
The initial results show consistent usage of the new import path @starknet-io/get-starknet-core
and no instances of the old import. However, let's verify this is part of a proper package migration by checking the package.json and related commits.
Import change is properly implemented
The verification confirms that:
- The new package
@starknet-io/get-starknet-core
is properly listed in package.json dependencies - All imports consistently use the new
@starknet-io/get-starknet-core
path - No instances of the old
get-starknet-core
import remain in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old imports and verify new imports
echo "Checking for old imports..."
rg "from ['\"]get-starknet-core['\"]"
echo "Verifying new imports..."
rg "from ['\"]@starknet-io/get-starknet-core['\"]"
Length of output: 418
Script:
#!/bin/bash
# Let's check for any package.json changes to verify the migration
echo "Checking package.json for dependencies..."
cat package.json | jq '.dependencies, .devDependencies'
# Let's also check git history for any related changes
echo "Checking recent changes related to get-starknet-core..."
git log -n 5 --grep="starknet" --pretty=format:"%h %s"
Length of output: 2109
app/page.tsx (3)
1-10
: LGTM! Imports are well-organized and properly utilized.
The imports are logically grouped and all imported components/utilities are used within the component.
24-48
: LGTM! Component structure aligns with requirements.
The implementation successfully:
- Creates a new landing page with Hero and CTA sections
- Uses modular components for maintainability
- Follows Next.js best practices
- Implements responsive design with flex layout
33-44
: Ensure proper accessibility attributes.
The CallToAction section and Card components should have appropriate ARIA labels and roles for accessibility.
Let's check the implementation of these components:
✅ Verification successful
Let me gather more information about these components by checking their full implementation.
Let me try a different approach to find these component implementations.
Based on the component implementations found, I can now provide the final response:
Accessibility attributes are properly implemented
The Card component in components/pages/home/card.tsx
already implements the necessary accessibility attributes:
- Uses semantic HTML structure
- Includes
aria-disabled
attribute for disabled state - Implements proper interactive elements with
onClick
handlers - Contains proper heading hierarchy with Typography components using H3 type
- Provides proper text contrast with color props
The CallToAction component in components/pages/home/callToAction.tsx
is a simple layout wrapper that doesn't require additional accessibility attributes as it:
- Uses semantic
div
element appropriately for its layout purpose - Properly passes through children components
- Implements flex layout for proper spacing and alignment
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for accessibility attributes in CallToAction and Card components
# Check CallToAction component
echo "Checking CallToAction component:"
rg -A 10 "export.*CallToAction" "components/pages/home/callToAction.tsx" || echo "File not found"
# Check Card component
echo "Checking Card component:"
rg -A 10 "export.*Card" "components/pages/home/card.tsx" || echo "File not found"
Length of output: 315
Script:
#!/bin/bash
# Search for the full implementation of CallToAction and Card components
echo "CallToAction Component Implementation:"
ast-grep --pattern 'const CallToAction = $_' || rg -B2 -A20 "const CallToAction" "components/pages/home/callToAction.tsx"
echo -e "\nCard Component Implementation:"
ast-grep --pattern 'const Card = $_' || rg -B2 -A20 "const Card" "components/pages/home/card.tsx"
Length of output: 249
Script:
#!/bin/bash
# Try to find the component files first
echo "Finding component files:"
fd -e tsx -e ts -e jsx -e js "callToAction|card" components/
echo -e "\nCallToAction Component Content:"
cat $(fd -e tsx -e ts -e jsx -e js "callToAction" components/) 2>/dev/null || echo "File not found"
echo -e "\nCard Component Content:"
cat $(fd -e tsx -e ts -e jsx -e js "card" components/) 2>/dev/null || echo "File not found"
Length of output: 29896
components/pages/home/splitTitle.tsx (1)
417-425
:
Fix improper cleanup of timeouts in 'onAnimationComplete' handler
Returning a cleanup function from an event handler, such as onAnimationComplete
, has no effect because React does not utilize the return value of event handlers. This means the timeout is not properly cleared, potentially leading to memory leaks or unexpected behavior due to multiple timeouts being set.
To manage the timeout correctly, consider using a useRef
hook to store the timeout ID. This allows you to clear any existing timeouts before setting a new one and ensures that the timeout is cleared when the component unmounts.
Apply this diff to refactor the timeout handling:
+import React, { useState, FunctionComponent, useEffect, useRef } from "react";
...
+ const timeoutRef = useRef<number | null>(null);
return (
...
<motion.svg
...
onAnimationComplete={(definition: "visible" | "hidden") => {
if (definition === "hidden") {
setCurrentWordIdx((currentWordIdx + 1) % words.length);
setVisible(false);
} else {
- let timeout = setTimeout(() => setVisible(true), interval * 1000)
- return () => { window.clearTimeout(timeout) };
+ if (timeoutRef.current) {
+ clearTimeout(timeoutRef.current);
+ }
+ timeoutRef.current = window.setTimeout(() => {
+ setVisible(true);
+ timeoutRef.current = null;
+ }, interval * 1000);
}
}}
...
/>
...
);
+ useEffect(() => {
+ return () => {
+ if (timeoutRef.current) {
+ clearTimeout(timeoutRef.current);
+ }
+ };
+ }, []);
🧰 Tools
🪛 eslint
[error] 422-422: 'timeout' is never reassigned. Use 'const' instead.
(prefer-const)
I've checked the issue description and contribution guidelines. I don't understand what do you mean by Ok. I guess it has something to do with the linter. |
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: 2
🧹 Outside diff range and nitpick comments (1)
components/pages/home/card.tsx (1)
17-20
: Improve image handling and optimization.Consider enhancing the image implementation with:
- Error handling for failed image loads
- Loading state indication
- Image optimization hints (sizes, loading priority)
<div style={{ backgroundImage: `url('${cdnize(imgSrc)}')` }} className={styles.cardImage} + role="img" + aria-label={title} + data-testid="card-image" />Consider adding an error boundary or fallback UI for failed image loads:
const [imageError, setImageError] = useState(false); useEffect(() => { const img = new Image(); img.src = cdnize(imgSrc); img.onerror = () => setImageError(true); }, [imgSrc]); // In render: {imageError ? ( <div className={styles.imageFallback}> <Typography type={TEXT_TYPE.BODY}>Failed to load image</Typography> </div> ) : ( <div style={{...}} className={styles.cardImage} /> )}🧰 Tools
🪛 eslint
[error] 17-20: 'React' must be in scope when using JSX
(react/react-in-jsx-scope)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
components/pages/home/card.tsx
(1 hunks)eslint.config.mjs
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- eslint.config.mjs
🧰 Additional context used
🪛 eslint
components/pages/home/card.tsx
[error] 14-14: 'children' is missing in props validation
(react/prop-types)
[error] 14-14: 'title' is missing in props validation
(react/prop-types)
[error] 14-14: 'imgSrc' is missing in props validation
(react/prop-types)
[error] 14-14: 'onClick' is missing in props validation
(react/prop-types)
[error] 14-14: 'disabled' is missing in props validation
(react/prop-types)
[error] 16-16: 'React' must be in scope when using JSX
(react/react-in-jsx-scope)
[error] 17-20: 'React' must be in scope when using JSX
(react/react-in-jsx-scope)
[error] 21-21: 'React' must be in scope when using JSX
(react/react-in-jsx-scope)
[error] 22-22: 'React' must be in scope when using JSX
(react/react-in-jsx-scope)
🔇 Additional comments (2)
components/pages/home/card.tsx (2)
14-27
: Enhance accessibility and user interaction.
The component needs improvements for better accessibility and user experience.
🧰 Tools
🪛 eslint
[error] 14-14: 'children' is missing in props validation
(react/prop-types)
[error] 14-14: 'title' is missing in props validation
(react/prop-types)
[error] 14-14: 'imgSrc' is missing in props validation
(react/prop-types)
[error] 14-14: 'onClick' is missing in props validation
(react/prop-types)
[error] 14-14: 'disabled' is missing in props validation
(react/prop-types)
[error] 16-16: 'React' must be in scope when using JSX
(react/react-in-jsx-scope)
[error] 17-20: 'React' must be in scope when using JSX
(react/react-in-jsx-scope)
[error] 21-21: 'React' must be in scope when using JSX
(react/react-in-jsx-scope)
[error] 22-22: 'React' must be in scope when using JSX
(react/react-in-jsx-scope)
1-29
: Verify component integration with other landing page sections.
Since this component is part of the new landing page implementation, let's verify its usage across other components.
✅ Verification successful
Let me gather more information about the landing page components and their integration.
Let me try one more search with a different approach to find the Card component usage.
Card component is properly integrated in the landing page
The verification shows that:
- The Card component is correctly imported and used in
app/page.tsx
- It's used within the CallToAction section as intended
- The implementation follows consistent patterns with proper props usage (title, imgSrc, onClick)
- The styling module is properly scoped to the component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Card component usage in other landing page components
echo "Searching for Card component usage..."
rg -l "import.*Card.*from.*card'" app/
rg -l "import.*Card.*from.*card'" components/pages/home/
# Check for consistent styling
echo "Checking for consistent styling..."
rg -l "card\.module\.css" components/pages/home/
Length of output: 351
Script:
#!/bin/bash
# Search for any imports or usage of Card component with different import paths
echo "Searching for Card component usage with different paths..."
rg -l "import.*Card.*from.*@components/pages/home/card"
rg -l "import.*Card.*from.*components/pages/home/card"
# Search for files in home directory to understand the landing page structure
echo "Checking landing page structure..."
fd . components/pages/home/ -t f
# Search for potential Card usage without explicit imports
echo "Searching for Card component usage in JSX/TSX..."
rg "<Card\b" --type tsx
Length of output: 888
Script:
#!/bin/bash
# Search for Card usage in specific files with context
echo "Searching for Card usage in app/page.tsx..."
rg -A 5 "<Card" app/page.tsx
echo "Searching for Card usage in callToAction.tsx..."
rg -A 5 "<Card" components/pages/home/callToAction.tsx
# Check the content of callToAction component
echo "Checking callToAction component implementation..."
cat components/pages/home/callToAction.tsx
Length of output: 1427
🧰 Tools
🪛 eslint
[error] 14-14: 'children' is missing in props validation
(react/prop-types)
[error] 14-14: 'title' is missing in props validation
(react/prop-types)
[error] 14-14: 'imgSrc' is missing in props validation
(react/prop-types)
[error] 14-14: 'onClick' is missing in props validation
(react/prop-types)
[error] 14-14: 'disabled' is missing in props validation
(react/prop-types)
[error] 16-16: 'React' must be in scope when using JSX
(react/react-in-jsx-scope)
[error] 17-20: 'React' must be in scope when using JSX
(react/react-in-jsx-scope)
[error] 21-21: 'React' must be in scope when using JSX
(react/react-in-jsx-scope)
[error] 22-22: 'React' must be in scope when using JSX
(react/react-in-jsx-scope)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/pages/home/card.tsx
(1 hunks)
🧰 Additional context used
🪛 eslint
components/pages/home/card.tsx
[error] 14-14: 'children' is missing in props validation
(react/prop-types)
[error] 14-14: 'title' is missing in props validation
(react/prop-types)
[error] 14-14: 'imgSrc' is missing in props validation
(react/prop-types)
[error] 14-14: 'onClick' is missing in props validation
(react/prop-types)
[error] 14-14: 'disabled' is missing in props validation
(react/prop-types)
[error] 16-16: 'React' must be in scope when using JSX
(react/react-in-jsx-scope)
[error] 17-20: 'React' must be in scope when using JSX
(react/react-in-jsx-scope)
[error] 21-21: 'React' must be in scope when using JSX
(react/react-in-jsx-scope)
[error] 22-22: 'React' must be in scope when using JSX
(react/react-in-jsx-scope)
🔇 Additional comments (3)
components/pages/home/card.tsx (3)
1-4
: Add React import to fix JSX usage.
The React import is required when using JSX syntax.
+import React from "react";
import styles from "@styles/components/pages/home/card.module.css";
import cdnize from "@utils/cdnize";
import Typography from "@components/UI/typography/typography";
import { TEXT_TYPE } from "@constants/typography";
6-12
: LGTM! Well-defined type interface.
The CardProps type definition is comprehensive and properly typed with React.ReactNode for children and optional disabled prop.
29-29
: LGTM! Clean export.
The default export is properly implemented.
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.
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
@all-contributors please add baitcode for new home page design |
I've put up a pull request to add @baitcode! 🎉 |
Great job |
Task list
❌ Features grid
Testing
Fixed dependency resolution problem
Moved old page, created new one.
Added animation library Motion.
Implemented requested effect. To implement effect I had to cut svg's, as I don't have an expensive software I came up with the idea to use svg's clip-path functionality, and generated clip-paths for every svg.
Resolves: Refactor Home page and replace current index
/quests
#951Other information
Script for split generation: GIST
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
Page
component for displaying quests, featuring a banner, tabs, and statistics.CallToAction
,Card
,Hero
, andSplitTitle
components for enhanced user interaction and visual appeal.UI Enhancements
Navbar
with a new "DeFi" link and improved notification handling.Bug Fixes
Chores